From 14734acc9d9e3e8350d6d45d8c62e0ec3463a6e4 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 11 Oct 2023 13:54:06 -0700 Subject: [PATCH] fix(core): Ensure backwards-referenced transplanted views are refreshed This commit runs change detection in a loop while there are still dirty views to be refreshed in the tree. At the moment, this only applies to transplanted views but will also apply to views with changed signals. fixes angular#49801 --- goldens/public-api/core/errors.md | 2 + packages/core/src/errors.ts | 1 + .../render3/instructions/change_detection.ts | 28 ++++++++++- ...change_detection_transplanted_view_spec.ts | 48 +++++++++---------- .../bundle.golden_symbols.json | 3 ++ .../animations/bundle.golden_symbols.json | 3 ++ .../cyclic_import/bundle.golden_symbols.json | 3 ++ .../bundling/defer/bundle.golden_symbols.json | 6 +++ .../forms_reactive/bundle.golden_symbols.json | 3 ++ .../bundle.golden_symbols.json | 3 ++ .../hello_world/bundle.golden_symbols.json | 3 ++ .../hydration/bundle.golden_symbols.json | 3 ++ .../router/bundle.golden_symbols.json | 3 ++ .../bundle.golden_symbols.json | 3 ++ .../bundling/todo/bundle.golden_symbols.json | 3 ++ 15 files changed, 87 insertions(+), 28 deletions(-) diff --git a/goldens/public-api/core/errors.md b/goldens/public-api/core/errors.md index 4aeefda8d794..f29525b2c1f2 100644 --- a/goldens/public-api/core/errors.md +++ b/goldens/public-api/core/errors.md @@ -57,6 +57,8 @@ export const enum RuntimeErrorCode { // (undocumented) IMPORT_PROVIDERS_FROM_STANDALONE = 800, // (undocumented) + INFINITE_CHANGE_DETECTION = 103, + // (undocumented) INJECTOR_ALREADY_DESTROYED = 205, // (undocumented) INVALID_DIFFER_INPUT = 900, diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index 331f765de694..0adf48ccf1cb 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -31,6 +31,7 @@ export const enum RuntimeErrorCode { EXPRESSION_CHANGED_AFTER_CHECKED = -100, RECURSIVE_APPLICATION_REF_TICK = 101, RECURSIVE_APPLICATION_RENDER = 102, + INFINITE_CHANGE_DETECTION = 103, // Dependency Injection Errors CYCLIC_DI_DEPENDENCY = -200, diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index a5f3423c1676..8fe630464d2d 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {RuntimeError, RuntimeErrorCode} from '../../errors'; import {assertDefined, assertEqual} from '../../util/assert'; import {assertLContainer} from '../assert'; import {getComponentViewByInstance} from '../context_discovery'; @@ -19,6 +20,11 @@ import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, mar import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCodes, refreshContentQueries} from './shared'; +/** + * The maximum number of times the change detection traversal will rerun before throwing an error. + */ +const MAXIMUM_REFRESH_RERUNS = 100; + export function detectChangesInternal( tView: TView, lView: LView, context: T, notifyErrorHandler = true) { const environment = lView[ENVIRONMENT]; @@ -37,6 +43,25 @@ export function detectChangesInternal( try { refreshView(tView, lView, tView.template, context); + let retries = 0; + // If after running change detection, this view still needs to be refreshed or there are + // descendants views that need to be refreshed due to re-dirtying during the change detection + // run, detect changes on the view again. We run change detection in `Targeted` mode to only + // refresh views with the `RefreshView` flag. + while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh)) { + if (retries === MAXIMUM_REFRESH_RERUNS) { + throw new RuntimeError( + RuntimeErrorCode.INFINITE_CHANGE_DETECTION, + ngDevMode && + 'Infinite change detection while trying to refresh views. ' + + 'There may be components which each cause the other to require a refresh, ' + + 'causing an infinite loop.'); + } + retries++; + // Even if this view is detached, we still detect changes in targeted mode because this was + // the root of the change detection run. + detectChangesInView(lView, ChangeDetectionMode.Targeted); + } } catch (error) { if (notifyErrorHandler) { handleError(lView, error); @@ -228,7 +253,6 @@ export function refreshView( if (!isInCheckNoChangesPass) { lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass); } - lView[FLAGS] &= ~LViewFlags.RefreshView; } catch (e) { // If refreshing a view causes an error, we need to remark the ancestors as needing traversal // because the error might have caused a situation where views below the current location are @@ -319,7 +343,7 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) { // Flag cleared before change detection runs so that the view can be re-marked for traversal if // necessary. - lView[FLAGS] &= ~LViewFlags.HasChildViewsToRefresh; + lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView); if ((flags & (LViewFlags.CheckAlways | LViewFlags.Dirty) && mode === ChangeDetectionMode.Global) || diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts index 3eba4179097f..600b92b2f187 100644 --- a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts +++ b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts @@ -410,35 +410,25 @@ describe('change detection for transplanted views', () => { expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); }); - it('updates only the declaration view when there is a change to declaration and declaration is marked dirty', - () => { - appComponent.declaration.name = 'new name'; - appComponent.declaration.changeDetectorRef.markForCheck(); - fixture.detectChanges(false); - - const expectedContent = - 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(new name)'; - expect(fixture.nativeElement.textContent).toEqual(expectedContent); - expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); + it('updates the declaration view when there is a change to either declaration or insertion', () => { + appComponent.declaration.name = 'new name'; + appComponent.declaration.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); - // Note here that this second change detection should not be necessary, but is because of - // the backwards reference not being fully supported. The assertions below should be true - // after the first CD. - fixture.detectChanges(false); - expect(fixture.nativeElement.textContent) - .toEqual( - 'Insertion(initial)TemplateDeclaration(new name)TemplateContext(initial)Declaration(new name)'); - expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); - }); + const expectedContent = + 'Insertion(initial)TemplateDeclaration(new name)TemplateContext(initial)Declaration(new name)'; + expect(fixture.nativeElement.textContent).toEqual(expectedContent); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); + }); - it('should not update when there is a change to insertion and declaration is marked dirty', () => { + it('should update when there is a change to insertion and declaration is marked dirty', () => { appComponent.insertion.name = 'new name'; appComponent.declaration.changeDetectorRef.markForCheck(); fixture.detectChanges(false); expect(fixture.nativeElement.textContent) .toEqual( 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(initial)'); - expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); }); it('should update insertion view and template when there is a change to insertion and insertion marked dirty', @@ -462,8 +452,9 @@ describe('change detection for transplanted views', () => { appComponent.insertion.changeDetectorRef.markForCheck(); fixture.detectChanges(false); expect(appComponent.declaration.transplantedViewRefreshCount) - .withContext('Should refresh once because backwards references are not rechecked') - .toEqual(1); + .withContext( + 'Should refresh twice because insertion executes and then declaration marks transplanted view dirty again') + .toEqual(2); }); }); @@ -653,7 +644,8 @@ describe('change detection for transplanted views', () => { // Detach view, manually call `detectChanges`, and verify the template was refreshed component.rootViewContainerRef.detach(); viewRef.detectChanges(); - expect(component.templateExecutions).toEqual(1); + // This view is a backwards reference so it's refreshed twice + expect(component.templateExecutions).toEqual(2); }); it('should work when change detecting detached transplanted view already marked for refresh', @@ -669,7 +661,8 @@ describe('change detection for transplanted views', () => { // should not affect parent counters. viewRef.detectChanges(); }).not.toThrow(); - expect(component.templateExecutions).toEqual(1); + // This view is a backwards reference so it's refreshed twice + expect(component.templateExecutions).toEqual(2); }); it('should work when re-inserting a previously detached transplanted view marked for refresh', @@ -689,7 +682,10 @@ describe('change detection for transplanted views', () => { // the counter, this would fail when attempted to decrement. fixture.detectChanges(false); }).not.toThrow(); - expect(component.templateExecutions).toBeGreaterThan(0); + // The transplanted view gets refreshed twice because it's actually inserted "backwards" + // The view is defined in AppComponent but inserted in its ViewContainerRef (as an + // embedded view in AppComponent's host view). + expect(component.templateExecutions).toEqual(2); }); it('should work when detaching an attached transplanted view with the refresh flag', () => { 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 4ca4f24c3d08..bf1869803216 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -833,6 +833,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index b388ab135e0f..ed8bae70b2c4 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -899,6 +899,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, 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 339526394f37..1b94eab2686f 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -668,6 +668,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index 66c87fefe52b..a70307e78288 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -248,6 +248,9 @@ { "name": "MATH_ML_NAMESPACE" }, + { + "name": "MAXIMUM_REFRESH_RERUNS" + }, { "name": "MINIMUM_SLOT" }, @@ -752,6 +755,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, 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 2541c49cac9a..c96bccee489c 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -911,6 +911,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, 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 c6f8a4f81f54..94a46354913f 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 @@ -881,6 +881,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, 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 abb62e44cc22..12b3954c8c11 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -527,6 +527,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index 4468b84fc621..95775b384467 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -725,6 +725,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 80b9dd7a73b9..fca3e5849afa 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1166,6 +1166,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, 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 9f8c2ecd9611..3bace25ebec6 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -596,6 +596,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index b1adad7c6d14..9cc524c38398 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -797,6 +797,9 @@ { "name": "detectChangesInEmbeddedViews" }, + { + "name": "detectChangesInView" + }, { "name": "detectChangesInViewIfAttached" },