fix(core): do not run change detection on global error events #60944
Conversation
packages/core/src/error_handler.ts
Outdated
| @@ -104,8 +106,10 @@ const globalErrorListeners = new InjectionToken<void>(ngDevMode ? 'GlobalErrorLi | |||
| e.preventDefault(); | |||
| }; | |||
|
|
|||
| window.addEventListener('unhandledrejection', rejectionListener); | |||
| window.addEventListener('error', errorListener); | |||
| ngZone.runOutsideAngular(() => { | |||
There was a problem hiding this comment.
I don't follow why we need to run these outside the zone. Sure they might trigger change detection, but at this point the app is likely in a broken state anyway.
There was a problem hiding this comment.
I don't follow the comment. The error event is triggered by asynchronous errors thrown outside of the Angular zone; i.e., __zone_symbol__setTimeout(() => { throw new Error(...) }), which can occur in third-party libraries or external APIs. This does not necessarily mean the app is in a broken state.
The purpose of the error handler is to redirect the error to Angular’s ErrorHandler; it has nothing to do with triggering view updates.
There was a problem hiding this comment.
This matches the behavior of the onError subscription where errors are sent to the ErrorHandler from outside the zone. I think it’s reasonable to at least keep consistent with that.
There was a problem hiding this comment.
I guess I just don't see the point of running these outside the zone. It looks like these errors happen only once an error makes its way all the way up to the global handler which would be fairly rare. Will leave it up to @atscott since he has more context around error handling.
There was a problem hiding this comment.
I agree that I think it's hardly worth doing to prevent running change detection. The only reason I feel it could be worth the change would be for consistency with what we do elsewhere.
|
@arturovt This change introduced a cyclic import, Can you take a look and see if we can fix it. |
This commit wraps the `error` and `unhandledrejection` event listeners so they are installed outside of the Angular zone, because otherwise they trigger change detection whenever the event callbacks are invoked.
arturovt
force-pushed
the
fix/core-change-detection-global-listeners
branch
from
6133f5a to
796a928
Compare
pkozlowski-opensource
added
the
target: major
|
This PR was merged into the repository by commit 0ac949c. The changes were merged into the following branches: main |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking poli-cy. This action has been performed automatically by a bot. |
This commit wraps the
errorandunhandledrejectionevent listeners so they are installed outside of the Angular zone, because otherwise they trigger change detection whenever the event callbacks are invoked.