-
Notifications
You must be signed in to change notification settings - Fork 77
fix error events not working #590
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
Conversation
|
/snapit |
|
🫰✨ Thanks @robin-drexler! Your snapshot has been published to npm. Test the snapshot by updating your "@remote-dom/polyfill": "0.0.0-snapshot-20250819185911" |
53795af to
3b9c61c
Compare
|
/snapit |
|
🫰✨ Thanks @robin-drexler! Your snapshot has been published to npm. Test the snapshot by updating your "@remote-dom/polyfill": "0.0.0-snapshot-20250819190822" |
3b9c61c to
201085b
Compare
packages/polyfill/source/Window.ts
Outdated
| MutationObserver = MutationObserver; | ||
|
|
||
| get onerror() { | ||
| return globalThis.onerror; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unsure if this would lead to infinite calls when we do setGlobal or setGlobalThis, but it worked for me when I tried it.
9186d8c to
82a63be
Compare
|
/snapit |
|
🫰✨ Thanks @robin-drexler! Your snapshot has been published to npm. Test the snapshot by updating your "@remote-dom/polyfill": "0.0.0-snapshot-20250821140812" |
| import {HOOKS} from './constants.ts'; | ||
| import type {Hooks} from './hooks.ts'; | ||
|
|
||
| type OnErrorHandler = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the original on error handler in libdom is typed weirdly in a way where the argument could either be an event object or the 5 args. But in reality it's only the 5 args. https://developer.mozilla.org/en-US/docs/Web/API/Window/error_event#syntax
So we're creating our own type
| if (this.#currentOnUnhandledRejectionHandler) { | ||
| this.removeEventListener( | ||
| 'unhandledrejection', | ||
| this.#currentOnUnhandledRejectionHandler as any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think add and remove listener on the polyfill isn't correctly typed. e.g. it doesn't know that unhandledrejection events should have an event object with promise and reason which is causing issues here. type casting as any for now
82a63be to
3e3f305
Compare
lemonmade
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
waseembahralaseel-cell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix my problem too
In polyfilled environments that use realms, it's currently not possible to listen to global errors and unhandled rejections.
onerror/onunhandledrejectionhandlerse.g.
the following works btw. but is not exhaustive enough because libs like sentry and bugsnag attach themselves to
windoworglobalThisWe now make this work by adding event listeners for those events the polyfill
windowinstance that just forwards it to theonerror/onunhandledrejectionfunctions. (thanks @igor10k for that idea!). Those events have been available for years so I think that's ok.This also doesn't work at the moment inside extensions:
This doesn't work because in realms the
globalThis.addEventListeneris not the one the worker is dispatching error events to. So we need to to dispatch those events manually from the worker to the realm global.However we can't dispatch the native event because the polyfill dispatchEvent implementation tries to override
targeton the event instance, which throws for native events. So we need to also polyfillErrorEventandPromiseRejectionEventwhich can be dispatched