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

ignoreErrors list #291

Closed
skovhus opened this issue Nov 15, 2014 · 5 comments
Closed

ignoreErrors list #291

skovhus opened this issue Nov 15, 2014 · 5 comments

Comments

@skovhus
Copy link

skovhus commented Nov 15, 2014

I made a comment back in #224 after it was merged. I guess you overlooked it, so I'll post it again:

I realize now that when adding a string to the whitelist filter, you don't start the regexp with a ^ or end it with $, e.g. the string can appear anywhere in the exception message. So these lines:

globalOptions.ignoreErrors.push('Script error.');
globalOptions.ignoreErrors.push('Script error');

are actually covering ... Javascript error: Script error on line ... error already. But also This is a script error that should be handled...... : /

Is this deliberately? It is rather confusing. That also means that my last pull request #224 was covered by the old rules.

@mattrobenolt
Copy link
Contributor

@skovhus Yeah, that is true.

Are you suggesting that it's problematic since script error is too general and may catch other things not related?

I was going to change it to this, but I see your point.

diff --git a/src/raven.js b/src/raven.js
index 6128169..d7bbbcb 100644
--- a/src/raven.js
+++ b/src/raven.js
@@ -76,12 +76,12 @@ var Raven = {

         // "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('Script error');
-
-        // Other variants of external script errors:
-        globalOptions.ignoreErrors.push('Javascript error: Script error on line 0');
-        globalOptions.ignoreErrors.push('Javascript error: Script error. on line 0');
+        // Variants include:
+        //   Script error.
+        //   Script error
+        //   Javascript error: Script error on line 0
+        //   Javascript error: Script error. on line 0
+        globalOptions.ignoreErrors.push('script error');

         // join regexp rules into one big rule
         globalOptions.ignoreErrors = joinRegExp(globalOptions.ignoreErrors);

@skovhus
Copy link
Author

skovhus commented Nov 16, 2014

Yes, it might hide too much.

So either we are explicit and make the four rules regular expressions. In my eyes that is the correct thing to do... We will not be hiding anything else that the four cases.

Else we could be less strict and just add Script error as a string. That would be more in line with the current behaviour. Very generic, but may hide some errors that should not be hidden.

@skovhus
Copy link
Author

skovhus commented Nov 16, 2014

A pull request for being more explicit #292

If you want to merge it, I will try it out on our site and check if we get some new external domain/CORS errors that the old less strict default ignoreErrors list filtered out.

@mattrobenolt
Copy link
Contributor

Fixed with #292

@skovhus
Copy link
Author

skovhus commented Nov 16, 2014

👍

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

No branches or pull requests

2 participants