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

Compare ignoreErrors pattern to error message #1076

Merged
merged 7 commits into from
Oct 10, 2017
Merged

Compare ignoreErrors pattern to error message #1076

merged 7 commits into from
Oct 10, 2017

Conversation

staticshock
Copy link
Contributor

This resumes filtering opaque "Script error" messages caused by errors in cross-origin scripts.

Fixes #1075.

@kamilogorek
Copy link
Contributor

@benvinegar this also resolves your concern from here #1057 (comment)

src/raven.js Outdated
this._globalOptions.ignoreErrors.test(testString)
!!this._globalOptions.ignoreErrors.test && (
this._globalOptions.ignoreErrors.test(message) ||
this._globalOptions.ignoreErrors.test(testString))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also change it so that testString doesn't become :message if it's undefined? per #1075 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Addressed.

Copy link
Contributor

@kamilogorek kamilogorek Oct 6, 2017

Choose a reason for hiding this comment

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

This code will double-check the same string if type is not defined, but I think we can live with it.

Although an alternative would be something like this, which also do the job, but doesn't improve a readability really

if (!!this._globalOptions.ignoreErrors.test) {
  var testString = (type ? type + ': ' : '') + (message || '')
  if (this._globalOptions.ignoreErrors.test(message)) return
  if (testString !== message && this._globalOptions.ignoreErrors.test(testString)) return
}

I can live with the code we have now.

@kamilogorek
Copy link
Contributor

kamilogorek commented Oct 6, 2017

@staticshock could you please add one quick test to make sure both scenarios work? https://github.com/getsentry/raven-js/blob/master/test/raven.test.js#L478-L490

Some of the scenarios where it should match:

input: 'CustomError: random 123 message'

/^CustomError: random 123 message$/
/^random 123 message$/
/^Custom Error/

'CustomError: random 123 message'
'123 message'
'CustomError'

@staticshock
Copy link
Contributor Author

@kamilogorek how do you feel about this?

@kamilogorek
Copy link
Contributor

kamilogorek commented Oct 9, 2017

@staticshock I think that in the end, initial version was more readable, so let's stick with it.
This double check issue I mentioned shouldn't be hit a lot of times anyway.
We can call it stringifiedError, errorString or something more descriptive than testString though :P

if (
  !!this._globalOptions.ignoreErrors.test && (
  this._globalOptions.ignoreErrors.test(message) ||
  this._globalOptions.ignoreErrors.test(stringifiedError))
)

Other then this all looks good! Thanks for updating tests :)

@staticshock
Copy link
Contributor Author

@kamilogorek kamilogorek merged commit b81a3f6 into getsentry:master Oct 10, 2017
@kamilogorek
Copy link
Contributor

Thanks! :)

@staticshock staticshock deleted the ignore-errors-original-recipe branch October 10, 2017 17:29
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.

2 participants