Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Include exception type in ignoreErrors tests #1057

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

kamilogorek
Copy link
Contributor

Ref #1005 and #1007

@kamilogorek kamilogorek requested review from benvinegar, MaxBittker and a team and removed request for benvinegar and MaxBittker September 26, 2017 13:08
@mitsuhiko
Copy link
Member

For what it's worth I think we should support that being a function instead of regexp optionally.

Copy link
Contributor

@benvinegar benvinegar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation would be that we append type and message together, and run the regex on that.

e.g. if you have type ReferenceError, and message foo is not defined, we'd run the regex against: ReferenceError: foo is not defined

No? What do other client libs do here? @mitsuhiko

@kamilogorek
Copy link
Contributor Author

@benvinegar only Ruby and Python have some sort of ignore options, and both of those SDKs use exception type only.

The reason why I didn't want to append them together, is that : in between. Although I'm almost sure that all the browsers show messages in the same type + ': ' + message format.

@kamilogorek kamilogorek changed the title feat: Include exception type in igoreErrors test feat: Include exception type in ignoreErrors test Sep 27, 2017
@kamilogorek kamilogorek changed the title feat: Include exception type in ignoreErrors test feat: Include exception type in ignoreErrors and allow function usage Sep 27, 2017
@kamilogorek
Copy link
Contributor Author

@benvinegar updated.
@mitsuhiko agree on the function usage, but it'd also require extracting global errors to a separate array, as they should always be used:

// "Script error." is hard coded into browsers for errors that it can't read.
// this is the result of a script being pulled in from an external domain and CORS.
globalOptions.ignoreErrors.push(/^Script error\.?$/);
globalOptions.ignoreErrors.push(/^Javascript error: Script error\.? on line 0$/);

Let's keep this feature in a separate issue.

@kamilogorek kamilogorek merged commit a023305 into master Sep 28, 2017
@kamilogorek kamilogorek deleted the ignoreerrors-type branch September 28, 2017 14:42
@kamilogorek kamilogorek changed the title feat: Include exception type in ignoreErrors and allow function usage feat: Include exception type in ignoreErrors tests Oct 2, 2017
@benvinegar
Copy link
Contributor

The reason why I didn't want to append them together, is that : in between.

Yeah, I hear you. But I think everyone just expected it to work this way (I for one thought it did).

@benvinegar
Copy link
Contributor

We should also keep in mind that if people were doing this: /^Strictly the message without type$/, those regexes will no longer work.

@kamilogorek
Copy link
Contributor Author

True that, I missed that scenario. Hopefully most of people are using string based tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants