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
is it necessary to monkey patch built-ins in *every* browser? #946
Comments
At the very least it's not entirely about "we need the error object". In some cases the errors that are made available simply are not usable, and this includes modern browsers like Firefox. For example, they may not include the full stack, or may be missing details (like column numbers). A big asterisk here in that I'm not sure this is blanketly true still today, but was not that long ago. tl;dr whats the harm in ensuring predictible behavior, then pretending browsers dont change/have quirks |
This is necessary largely to ensure that the same error results in the exact same stack traces across different browsers, so that we can accurately group multiple occurrences of the same issue together across different browsers. This also relates to how we get more than just "Script Error" when cross-origin scripts throw errors (see Ben's blog post on that) and we collect some of our automatic breadcrumbs from this instrumentation. All that aside, the potential "unnecessary work" you describe is extremely minimal, feature detecting |
I should add that without try/catch, we can't generate a "synthetic trace" in cases where a non-error object is thrown (e.g. |
Reading the intro at https://blog.sentry.io/2016/01/04/client-javascript-reporting-window-onerror.html and looking at source code of raven, is really necessary to polyfill the
onerror
by monkey patching every built-in with try/catch in order to retrieve the stack trace, even if given browser (e.g. chrome/FF) supplies the error object?To me it seems like forcing the browser to do unnecessary work if it doesn't have to. Or is raven doing something else that warrants this?
Edit: probably not, since you don't seem to be doing it for the raven-node.
The text was updated successfully, but these errors were encountered: