Reasoning behind using window.onerror over window.addEventListener('error')

[simenbrekken-visma]

Yesterday our Sentry reporting stopped working for uncaught exceptions as someone had added a window.onerror somewhere in our pretty large code base. The obvious solution is naturally not writing to window.onerror but trying to track down why things were not working I noticed that the GlobalHandlers integration uses window.onerror that can be overridden at any point in time by either user, library or even extension code (which was exactly what happened).

Why isn't Sentry using window.addEventListener('error')?

[AbhiPrasad]

Hey! Super interesting question. I asked this to @kamilogorek on our internal slack, and I'll summarize his answer here:

Right now it doesn't make that much a difference (if any). If I recall correctly, it was due to the fact that we wanted to be registered first (that's one thing); although now it doesn't work with new event listeners, as onerror is just syntax sugar. However, onerror is necessary for other usecases, outside of the main SDK

Mainly, these use cases are so that we can access the event listener after they have been set, for things like our async loader.

// fails
window.addEventListener('error', () => {})
console.log(window.onerror)

// works
window.onerror = () => {}
console.log(window.onerror)

In addition, window.onerror just extends the browser compat opportunities, and is tested pretty heavily with our codebase, so the change isn't super worth it for us. Even if our package emits es6, we want the ability for people to be able to downcompile the code to es5 or below if they need to support older browser versions, so we need to still support those older browser APIs.

[simenbrekken-visma]

Could be worth something about this to the troubleshooting documentation but I understand your reasoning.

[simenbrekken-visma]

Also, looking at your example couldn't you do the following?

// works and can't be overridden
window.__SENTRY__onerror = () => {};
window.addEventListener('error', window.__SENTRY__onerror);
console.log(window.__SENTRY__onerror);

[kamilogorek]

Yes, that'd work as well with some changes I guess. It's just always not easy to modify a behavior that's there for almost 5 years without having a feeling that it can break in one of some obscure browsers (and we had a looot of those, including some Chinese vendored browsers 😅)

btw. if you are interested, here's loaders code https://github.com/getsentry/sentry/blob/master/src/sentry/templates/sentry/js-sdk-loader.js.tmpl

[chenxin1212]

Hi, all.

I also happened to be thinking about this recently. there's another benefit to make this change. it can help to capture resource errors such as missing images or scripts.

window.addEventListener('error', () => {}, true)

[AbhiPrasad]

it can help to capture resource errors such as missing images or scripts

We purposefully do not capture these resource errors - you can manually instrument this though! See our docs about this: https://docs.sentry.io/platforms/javascript/troubleshooting/#capturing-resource-404s