Don't report errors errors as unhandled if there is an existing "error" event handler #4767
Conversation
|
Heh, we fixed lint at the same time |
|
Bah, rest args is not available on node 4. Needs |
|
Lol. I pushed a commit that tracks the number of user handlers. I think it's more correct because if the user removes the listener, we should restore the original behavior. |
gaearon
changed the title
| if (name === 'error') { | ||
| userErrorListenerCount--; | ||
| } | ||
| return originalRemoveListener.apply(this, arguments); |
There was a problem hiding this comment.
should be .apply(global, arguments) as this is wrong in the arrow function, right?
arguments is also wrong within the arrow
| this.errorEventListener = event => { | ||
| if (event.error) { | ||
| if (userErrorListenerCount === 0 && event.error) { |
There was a problem hiding this comment.
Instead of manual bookkeeping, can we just check the total number of listeners here?
e.g. this works in chrome:
window.getEventListeners(window).error.length // 1 on github.comThere was a problem hiding this comment.
Looks like getEventListeners is not available for jsdom@9 at least
EDIT: Or in 11
There was a problem hiding this comment.
I'm fine with upgrading to 11 and dropping node 4 now.
There was a problem hiding this comment.
Seems to be a devtools API, so never mind!
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This add a failing integration test for behavior React relies on.
It passes before #4669, but now fails.
DemonstratesFixes #4764.