From 7df133dcc243cd6b0f779fa35de7f916e6938301 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 24 Jan 2024 15:02:18 -0800 Subject: [PATCH] fix(core): `afterRender` hooks should allow updating state (#54074) It was always the intent to have `afterRender` hooks allow updating state, as long as those state updates specifically mark views for (re)check. This commit delivers that behavior. Developers can now use `afterRender` hooks to perform further updates within the same change detection cycle rather than needing to defer this work to another round (i.e. `queueMicrotask(() => <>)`). This is an important change to support migrating from the `queueMicrotask`-style deferred updates, which are not entirely compatible with zoneless or event `NgZone` run coalescing. When using a microtask to update state after a render, it doesn't work with coalescing because the render may not have happened yet (coalescing and zoneless use `requestAnimationFrame` while the default for `NgZone` is effectively a microtask-based change detection scheduler). Second, when using a `microtask` _during_ change detection to defer updates to the next cycle, this can cause updates to be split across multiple frames with run coalescing and with zoneless (again, because of `requestAnimationFrame`/`macrotask` change detection scheduling). PR Close #54074 --- .../core/src/application/application_ref.ts | 62 +++++++++++- .../render3/instructions/change_detection.ts | 13 +-- packages/core/src/render3/util/view_utils.ts | 5 +- packages/core/src/render3/view_ref.ts | 2 +- .../test/acceptance/after_render_hook_spec.ts | 94 ++++++++++++++++++- .../change_detection_signals_in_zones_spec.ts | 30 ------ .../bundle.golden_symbols.json | 6 ++ .../animations/bundle.golden_symbols.json | 6 ++ .../cyclic_import/bundle.golden_symbols.json | 6 ++ .../bundling/defer/bundle.golden_symbols.json | 6 ++ .../forms_reactive/bundle.golden_symbols.json | 6 ++ .../bundle.golden_symbols.json | 6 ++ .../hello_world/bundle.golden_symbols.json | 6 ++ .../hydration/bundle.golden_symbols.json | 6 ++ .../router/bundle.golden_symbols.json | 6 ++ .../bundle.golden_symbols.json | 6 ++ .../bundling/todo/bundle.golden_symbols.json | 6 ++ 17 files changed, 228 insertions(+), 44 deletions(-) diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts index 98471cb8bf4f0..66916e73f970d 100644 --- a/packages/core/src/application/application_ref.ts +++ b/packages/core/src/application/application_ref.ts @@ -33,9 +33,12 @@ import {AfterRenderEventManager} from '../render3/after_render_hooks'; import {assertNgModuleType} from '../render3/assert'; import {ComponentFactory as R3ComponentFactory} from '../render3/component_ref'; import {isStandalone} from '../render3/definition'; +import {ChangeDetectionMode, detectChangesInternal, MAXIMUM_REFRESH_RERUNS} from '../render3/instructions/change_detection'; +import {FLAGS, LView, LViewFlags} from '../render3/interfaces/view'; import {setJitOptions} from '../render3/jit/jit_options'; import {NgModuleFactory as R3NgModuleFactory} from '../render3/ng_module_ref'; import {publishDefaultGlobalUtils as _publishDefaultGlobalUtils} from '../render3/util/global_utils'; +import {requiresRefreshOrTraversal} from '../render3/util/view_utils'; import {ViewRef as InternalViewRef} from '../render3/view_ref'; import {TESTABILITY} from '../testability/testability'; import {isPromise} from '../util/lang'; @@ -546,10 +549,9 @@ export class ApplicationRef { try { this._runningTick = true; - for (let view of this._views) { - view.detectChanges(); - } - this.afterRenderEffectManager.execute(); + + this.detectChangesInAttachedViews(); + if ((typeof ngDevMode === 'undefined' || ngDevMode)) { for (let view of this._views) { view.checkNoChanges(); @@ -563,6 +565,53 @@ export class ApplicationRef { } } + private detectChangesInAttachedViews() { + let runs = 0; + do { + if (runs === MAXIMUM_REFRESH_RERUNS) { + throw new RuntimeError( + RuntimeErrorCode.INFINITE_CHANGE_DETECTION, + ngDevMode && + 'Changes in afterRender or afterNextRender hooks caused infinite change detection while refresh views.'); + } + + const isFirstPass = runs === 0; + for (let {_lView, notifyErrorHandler} of this._views) { + // When re-checking, only check views which actually need it. + if (!isFirstPass && !shouldRecheckView(_lView)) { + continue; + } + this.detectChangesInView(_lView, notifyErrorHandler, isFirstPass); + } + this.afterRenderEffectManager.execute(); + + runs++; + } while (this._views.some(({_lView}) => shouldRecheckView(_lView))); + } + + private detectChangesInView(lView: LView, notifyErrorHandler: boolean, isFirstPass: boolean) { + let mode: ChangeDetectionMode; + if (isFirstPass) { + // The first pass is always in Global mode, which includes `CheckAlways` views. + mode = ChangeDetectionMode.Global; + // Add `RefreshView` flag to ensure this view is refreshed if not already dirty. + // `RefreshView` flag is used intentionally over `Dirty` because it gets cleared before + // executing any of the actual refresh code while the `Dirty` flag doesn't get cleared + // until the end of the refresh. Using `RefreshView` prevents creating a potential + // difference in the state of the LViewFlags during template execution. + lView[FLAGS] |= LViewFlags.RefreshView; + } else if (lView[FLAGS] & LViewFlags.Dirty) { + // The root view has been explicitly marked for check, so check it in Global mode. + mode = ChangeDetectionMode.Global; + } else { + // The view has not been marked for check, but contains a view marked for refresh + // (likely via a signal). Start this change detection in Targeted mode to skip the root + // view and check just the view(s) that need refreshed. + mode = ChangeDetectionMode.Targeted; + } + detectChangesInternal(lView, notifyErrorHandler, mode); + } + /** * Attaches a view so that it will be dirty checked. * The view will be automatically detached when it is destroyed. @@ -715,3 +764,8 @@ export function whenStable(applicationRef: ApplicationRef): Promise { return whenStablePromise; } + + +function shouldRecheckView(view: LView): boolean { + return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty); +} diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index e4932777c6cd7..8dfae64622d3e 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -25,9 +25,10 @@ import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCo /** * The maximum number of times the change detection traversal will rerun before throwing an error. */ -const MAXIMUM_REFRESH_RERUNS = 100; +export const MAXIMUM_REFRESH_RERUNS = 100; -export function detectChangesInternal(lView: LView, notifyErrorHandler = true) { +export function detectChangesInternal( + lView: LView, notifyErrorHandler = true, mode = ChangeDetectionMode.Global) { const environment = lView[ENVIRONMENT]; const rendererFactory = environment.rendererFactory; @@ -41,7 +42,7 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) { } try { - detectChangesInViewWhileDirty(lView); + detectChangesInViewWhileDirty(lView, mode); } catch (error) { if (notifyErrorHandler) { handleError(lView, error); @@ -58,8 +59,8 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) { } } -function detectChangesInViewWhileDirty(lView: LView) { - detectChangesInView(lView, ChangeDetectionMode.Global); +function detectChangesInViewWhileDirty(lView: LView, mode: ChangeDetectionMode) { + detectChangesInView(lView, mode); let retries = 0; // If after running change detection, this view still needs to be refreshed or there are @@ -99,7 +100,7 @@ export function checkNoChangesInternal(lView: LView, notifyErrorHandler = true) * The change detection traversal algorithm switches between these modes based on various * conditions. */ -const enum ChangeDetectionMode { +export const enum ChangeDetectionMode { /** * In `Global` mode, `Dirty` and `CheckAlways` views are refreshed as well as views with the * `RefreshView` flag. diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index 81b29ee031423..31dd5a09fc5e7 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -197,8 +197,9 @@ export function walkUpViews(nestingLevel: number, currentView: LView): LView { } export function requiresRefreshOrTraversal(lView: LView) { - return lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) || - lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty; + return !!( + lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) || + lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty); } diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 8e97f873a1ced..6a39a9f577d1e 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -57,7 +57,7 @@ export class ViewRef implements EmbeddedViewRef, ChangeDetectorRefInterfac * * This may be different from `_lView` if the `_cdRefInjectingView` is an embedded view. */ - private _cdRefInjectingView?: LView, private readonly notifyErrorHandler = true) {} + private _cdRefInjectingView?: LView, readonly notifyErrorHandler = true) {} get context(): T { return this._lView[CONTEXT] as unknown as T; diff --git a/packages/core/test/acceptance/after_render_hook_spec.ts b/packages/core/test/acceptance/after_render_hook_spec.ts index 0028d17ab5f94..584f4af825b8b 100644 --- a/packages/core/test/acceptance/after_render_hook_spec.ts +++ b/packages/core/test/acceptance/after_render_hook_spec.ts @@ -7,7 +7,7 @@ */ import {PLATFORM_BROWSER_ID, PLATFORM_SERVER_ID} from '@angular/common/src/platform_id'; -import {afterNextRender, afterRender, AfterRenderPhase, AfterRenderRef, ApplicationRef, ChangeDetectorRef, Component, computed, createComponent, effect, ErrorHandler, inject, Injector, NgZone, PLATFORM_ID, Type, ViewContainerRef, ɵinternalAfterNextRender as internalAfterNextRender} from '@angular/core'; +import {afterNextRender, afterRender, AfterRenderPhase, AfterRenderRef, ApplicationRef, ChangeDetectorRef, Component, computed, createComponent, effect, ErrorHandler, inject, Injector, NgZone, PLATFORM_ID, signal, Type, ViewContainerRef, ɵinternalAfterNextRender as internalAfterNextRender} from '@angular/core'; import {NoopNgZone} from '@angular/core/src/zone/ng_zone'; import {TestBed} from '@angular/core/testing'; @@ -894,6 +894,98 @@ describe('after render hooks', () => { ]); }); }); + + it('allows writing to a signal in afterRender', () => { + const counter = signal(0); + + @Component({ + selector: 'test-component', + standalone: true, + template: ` {{counter()}} `, + }) + class TestCmp { + counter = counter; + injector = inject(EnvironmentInjector); + ngOnInit() { + afterNextRender(() => { + this.counter.set(1); + }, {injector: this.injector}); + } + } + TestBed.configureTestingModule( + {providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]}); + + const fixture = TestBed.createComponent(TestCmp); + const appRef = TestBed.inject(ApplicationRef); + appRef.attachView(fixture.componentRef.hostView); + appRef.tick(); + expect(fixture.nativeElement.innerText).toBe('1'); + }); + + it('allows updating state and calling markForCheck in afterRender', () => { + @Component({ + selector: 'test-component', + standalone: true, + template: ` {{counter}} `, + }) + class TestCmp { + counter = 0; + injector = inject(EnvironmentInjector); + cdr = inject(ChangeDetectorRef); + ngOnInit() { + afterNextRender(() => { + this.counter = 1; + this.cdr.markForCheck(); + }, {injector: this.injector}); + } + } + TestBed.configureTestingModule( + {providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]}); + + const fixture = TestBed.createComponent(TestCmp); + const appRef = TestBed.inject(ApplicationRef); + appRef.attachView(fixture.componentRef.hostView); + appRef.tick(); + expect(fixture.nativeElement.innerText).toBe('1'); + }); + + it('throws error when causing infinite updates', () => { + const counter = signal(0); + + @Component({ + selector: 'test-component', + standalone: true, + template: ` {{counter()}} `, + }) + class TestCmp { + counter = counter; + injector = inject(EnvironmentInjector); + ngOnInit() { + afterRender(() => { + this.counter.update(v => v + 1); + }, {injector: this.injector}); + } + } + TestBed.configureTestingModule({ + providers: [ + {provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}, + { + provide: ErrorHandler, useClass: class extends ErrorHandler { + override handleError(error: any) { + throw error; + } + } + }, + ] + }); + + const fixture = TestBed.createComponent(TestCmp); + const appRef = TestBed.inject(ApplicationRef); + appRef.attachView(fixture.componentRef.hostView); + expect(() => { + appRef.tick(); + }).toThrowError(/NG0103.*(afterRender|afterNextRender)/); + }); }); describe('server', () => { diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts index 5cd2df106a240..c3ac5bef4a5af 100644 --- a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts +++ b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts @@ -887,36 +887,6 @@ describe('OnPush components with signals', () => { expect(fixture.nativeElement.innerText).toEqual('2'); }); - // Note: We probably don't want this to throw but need to decide how to handle reruns - // This asserts current behavior and should be updated when this is handled - it('throws error when writing to a signal in afterRender', () => { - const counter = signal(0); - - @Component({ - selector: 'test-component', - standalone: true, - template: ` {{counter()}} `, - }) - class TestCmp { - counter = counter; - injector = inject(EnvironmentInjector); - ngOnInit() { - afterNextRender(() => { - this.counter.set(1); - }, {injector: this.injector}); - } - } - TestBed.configureTestingModule( - {providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]}); - - const fixture = TestBed.createComponent(TestCmp); - const appRef = TestBed.inject(ApplicationRef); - appRef.attachView(fixture.componentRef.hostView); - const spy = spyOn(console, 'error'); - appRef.tick(); - expect(spy.calls.first().args[1]).toMatch(/ExpressionChanged/); - }); - it('destroys all signal consumers when destroying the view tree', () => { const val = signal(1); const double = computed(() => val() * 2); 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 df6a194cedccc..5c700f95c8ebb 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -797,6 +797,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1352,6 +1355,9 @@ { "name": "shimStylesContent" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index 4adc593faa72e..11032f793e624 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -863,6 +863,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1424,6 +1427,9 @@ { "name": "shimStylesContent" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, 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 a1949b70abda5..7a06e2561bea6 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -647,6 +647,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1130,6 +1133,9 @@ { "name": "shimStylesContent" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index 67327add7b340..24804ffdd1cb3 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -740,6 +740,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -2249,6 +2252,9 @@ { "name": "shimStylesContent" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, 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 da0b03bcc5753..62c242dad84c7 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -905,6 +905,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1661,6 +1664,9 @@ { "name": "shouldAddViewToDom" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, 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 56fe1b61ef688..6a32b999fa95b 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 @@ -875,6 +875,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1637,6 +1640,9 @@ { "name": "shouldAddViewToDom" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, 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 c1edeb8730065..4a18f874c4f4c 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -506,6 +506,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -881,6 +884,9 @@ { "name": "setUpAttributes" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index 42c337a07ce83..e79edf9c0c9b7 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -707,6 +707,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1250,6 +1253,9 @@ { "name": "shimStylesContent" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 10629a5ad9378..364d3fa24d82f 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1088,6 +1088,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1922,6 +1925,9 @@ { "name": "shouldAddViewToDom" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, 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 91f696887e0bf..258221054299f 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -575,6 +575,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -983,6 +986,9 @@ { "name": "shimStylesContent" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 5a9bda9a21fe0..c796f3674f9f6 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -779,6 +779,9 @@ { "name": "detectChangesInViewIfAttached" }, + { + "name": "detectChangesInternal" + }, { "name": "diPublicInInjector" }, @@ -1376,6 +1379,9 @@ { "name": "shouldAddViewToDom" }, + { + "name": "shouldRecheckView" + }, { "name": "shouldSearchParent" },