From 491b0a4ead98822c767543e1f1c8046ed9d1be20 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 21 Feb 2025 11:35:31 -0800 Subject: [PATCH] fix(core): Remove duplicate reporting of errors in `CDR.detectChanges` (#60056) This change removes the reporting of errors from the `ChangeDetectorRef.detectChanges` API. The reporting results in the error being "handled" in two ways, both by reporting to error handler and rethrowing the error. This rethrown error generally ends up being caught further up and again reported to the error handler. The error handler is meant to be for uncaught errors, and since Angular is not at the top of the stack of the call of `CDR.detectChanges`, it does not know what is being done with the rethrown error. Note that for zone-based applications, this will likely have no effect other than removing duplicate reporting of the error. If the rethrown error is not already being caught, it will reach the NgZone's error trap and still be reported to the application `ErrorHandler`. PR Close #60056 --- .../core/src/application/application_ref.ts | 12 +++-------- .../scheduling/exhaustive_check_no_changes.ts | 4 ++-- packages/core/src/render3/component_ref.ts | 1 - .../render3/instructions/change_detection.ts | 21 ++++--------------- packages/core/src/render3/view_ref.ts | 9 ++------ .../bundle.golden_symbols.json | 1 - .../animations/bundle.golden_symbols.json | 1 - .../cyclic_import/bundle.golden_symbols.json | 1 - .../bundling/defer/bundle.golden_symbols.json | 1 - .../forms_reactive/bundle.golden_symbols.json | 2 -- .../bundle.golden_symbols.json | 2 -- .../hello_world/bundle.golden_symbols.json | 1 - .../hydration/bundle.golden_symbols.json | 1 - .../router/bundle.golden_symbols.json | 2 -- .../bundle.golden_symbols.json | 1 - .../bundling/todo/bundle.golden_symbols.json | 2 -- 16 files changed, 11 insertions(+), 51 deletions(-) diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts index 3fe4a7d3f4b..df1cc51e972 100644 --- a/packages/core/src/application/application_ref.ts +++ b/packages/core/src/application/application_ref.ts @@ -669,13 +669,8 @@ export class ApplicationRef { this.dirtyFlags |= ApplicationRefDirtyFlags.AfterRender; // Check all potentially dirty views. - for (let {_lView, notifyErrorHandler} of this.allViews) { - detectChangesInViewIfRequired( - _lView, - notifyErrorHandler, - useGlobalCheck, - this.zonelessEnabled, - ); + for (let {_lView} of this.allViews) { + detectChangesInViewIfRequired(_lView, useGlobalCheck, this.zonelessEnabled); } // If `markForCheck()` was called during view checking, it will have set the `ViewTreeCheck` @@ -893,7 +888,6 @@ export const enum ApplicationRefDirtyFlags { export function detectChangesInViewIfRequired( lView: LView, - notifyErrorHandler: boolean, isFirstPass: boolean, zonelessEnabled: boolean, ) { @@ -910,5 +904,5 @@ export function detectChangesInViewIfRequired( ChangeDetectionMode.Global : // Only refresh views with the `RefreshView` flag or views is a changed signal ChangeDetectionMode.Targeted; - detectChangesInternal(lView, notifyErrorHandler, mode); + detectChangesInternal(lView, mode); } diff --git a/packages/core/src/change_detection/scheduling/exhaustive_check_no_changes.ts b/packages/core/src/change_detection/scheduling/exhaustive_check_no_changes.ts index 9b2adc61e72..99b6b7ba10b 100644 --- a/packages/core/src/change_detection/scheduling/exhaustive_check_no_changes.ts +++ b/packages/core/src/change_detection/scheduling/exhaustive_check_no_changes.ts @@ -117,7 +117,7 @@ export class DebugNgZoneForCheckNoChanges extends NgZone { this.applicationRef ||= this.injector.get(ApplicationRef); for (const view of this.applicationRef.allViews) { try { - checkNoChangesInternal(view._lView, this.checkNoChangesMode, view.notifyErrorHandler); + checkNoChangesInternal(view._lView, this.checkNoChangesMode); } catch (e) { this.errorHandler ||= this.injector.get(ErrorHandler); this.errorHandler.handleError(e); @@ -153,7 +153,7 @@ function exhaustiveCheckNoChangesInterval( for (const view of applicationRef.allViews) { try { - checkNoChangesInternal(view._lView, checkNoChangesMode, view.notifyErrorHandler); + checkNoChangesInternal(view._lView, checkNoChangesMode); } catch (e) { errorHandler.handleError(e); } diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index 0e961834ba3..3ac783fad51 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -374,7 +374,6 @@ export class ComponentRef extends AbstractComponentRef { this.hostView = this.changeDetectorRef = new ViewRef( _rootLView, undefined /* _cdRefInjectingView */, - false /* notifyErrorHandler */, ); this.componentType = componentType; } diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 406736d6315..65e156af010 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -69,18 +69,14 @@ import {profiler} from '../profiler'; import {ProfilerEvent} from '../profiler_types'; import {executeViewQueryFn, refreshContentQueries} from '../queries/query_execution'; import {runEffectsInView} from '../reactivity/view_effect_runner'; -import {executeTemplate, handleError} from './shared'; +import {executeTemplate} from './shared'; /** * The maximum number of times the change detection traversal will rerun before throwing an error. */ export const MAXIMUM_REFRESH_RERUNS = 100; -export function detectChangesInternal( - lView: LView, - notifyErrorHandler = true, - mode = ChangeDetectionMode.Global, -) { +export function detectChangesInternal(lView: LView, mode = ChangeDetectionMode.Global) { const environment = lView[ENVIRONMENT]; const rendererFactory = environment.rendererFactory; @@ -95,11 +91,6 @@ export function detectChangesInternal( try { detectChangesInViewWhileDirty(lView, mode); - } catch (error) { - if (notifyErrorHandler) { - handleError(lView, error); - } - throw error; } finally { if (!checkNoChangesMode) { rendererFactory.end?.(); @@ -146,14 +137,10 @@ function detectChangesInViewWhileDirty(lView: LView, mode: ChangeDetectionMode) } } -export function checkNoChangesInternal( - lView: LView, - mode: CheckNoChangesMode, - notifyErrorHandler = true, -) { +export function checkNoChangesInternal(lView: LView, mode: CheckNoChangesMode) { setIsInCheckNoChangesMode(mode); try { - detectChangesInternal(lView, notifyErrorHandler); + detectChangesInternal(lView); } finally { setIsInCheckNoChangesMode(CheckNoChangesMode.Off); } diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index b3fde05ce82..0b36f10ea11 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -74,7 +74,6 @@ export class ViewRef implements EmbeddedViewRef, ChangeDetectorRefInterfac * This may be different from `_lView` if the `_cdRefInjectingView` is an embedded view. */ private _cdRefInjectingView?: LView, - readonly notifyErrorHandler = true, ) {} get context(): T { @@ -327,7 +326,7 @@ export class ViewRef implements EmbeddedViewRef, ChangeDetectorRefInterfac // until the end of the refresh. Using `RefreshView` prevents creating a potential difference // in the state of the LViewFlags during template execution. this._lView[FLAGS] |= LViewFlags.RefreshView; - detectChangesInternal(this._lView, this.notifyErrorHandler); + detectChangesInternal(this._lView); } /** @@ -338,11 +337,7 @@ export class ViewRef implements EmbeddedViewRef, ChangeDetectorRefInterfac */ checkNoChanges(): void { if (ngDevMode) { - checkNoChangesInternal( - this._lView, - CheckNoChangesMode.OnlyDirtyViews, - this.notifyErrorHandler, - ); + checkNoChangesInternal(this._lView, CheckNoChangesMode.OnlyDirtyViews); } } diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json index 26cfac1e072..643e999a76f 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -270,7 +270,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "documentElement", diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index 3cbef4bdeda..6f228f4aaed 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -291,7 +291,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "documentElement", diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index ca2ace50be3..6d7ef31f995 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -223,7 +223,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "elementEndFirstCreatePass", diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index 6deb50f892b..00ee7af65e2 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -271,7 +271,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "elementEndFirstCreatePass", diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json index 317af64bc59..2b55062eb1d 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -324,7 +324,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "elementEndFirstCreatePass", @@ -408,7 +407,6 @@ "getTStylingRangePrev", "getTView", "getViewRefs", - "handleError", "handleStoppedNotification", "handleUnhandledError", "hasApplyArgsData", diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json index ad2c8af42ec..df6e985b241 100644 --- a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json @@ -312,7 +312,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "elementEndFirstCreatePass", @@ -395,7 +394,6 @@ "getTStylingRangePrev", "getTView", "getViewRefs", - "handleError", "handleStoppedNotification", "handleUnhandledError", "hasApplyArgsData", diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index bbaca3372b0..0d4eafe919c 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -176,7 +176,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "enterDI", diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index 6951097930a..cc412f78dec 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -233,7 +233,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "enterDI", diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 247a62a1971..33447de9f2c 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -380,7 +380,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "elementEndFirstCreatePass", @@ -483,7 +482,6 @@ "getTView", "getTokenOrFunctionIdentity", "getViewRefs", - "handleError", "handleStoppedNotification", "handleUnhandledError", "hasApplyArgsData", diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json index 3f9bb34b608..8716d06e8a7 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -202,7 +202,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "enterDI", diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 454d11002e9..82298998c80 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -264,7 +264,6 @@ "detectChangesInEmbeddedViews", "detectChangesInView", "detectChangesInViewIfAttached", - "detectChangesInViewIfRequired", "detectChangesInternal", "diPublicInInjector", "elementEndFirstCreatePass", @@ -337,7 +336,6 @@ "getTStylingRangePrev", "getTView", "getViewRefs", - "handleError", "handleStoppedNotification", "handleUnhandledError", "hasApplyArgsData",