Skip to content

Commit 7df133d

Browse files
atscottthePunderWoman
atscott
authored and
thePunderWoman
committed
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(() => <<updateState>>)`). 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
1 parent 2a0ac20 commit 7df133d

File tree

17 files changed

+228
-44
lines changed

17 files changed

+228
-44
lines changed

‎packages/core/src/application/application_ref.ts‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,12 @@ import {AfterRenderEventManager} from '../render3/after_render_hooks';
3333
import {assertNgModuleType} from '../render3/assert';
3434
import {ComponentFactory as R3ComponentFactory} from '../render3/component_ref';
3535
import {isStandalone} from '../render3/definition';
36+
import {ChangeDetectionMode, detectChangesInternal, MAXIMUM_REFRESH_RERUNS} from '../render3/instructions/change_detection';
37+
import {FLAGS, LView, LViewFlags} from '../render3/interfaces/view';
3638
import {setJitOptions} from '../render3/jit/jit_options';
3739
import {NgModuleFactory as R3NgModuleFactory} from '../render3/ng_module_ref';
3840
import {publishDefaultGlobalUtils as _publishDefaultGlobalUtils} from '../render3/util/global_utils';
41+
import {requiresRefreshOrTraversal} from '../render3/util/view_utils';
3942
import {ViewRef as InternalViewRef} from '../render3/view_ref';
4043
import {TESTABILITY} from '../testability/testability';
4144
import {isPromise} from '../util/lang';
@@ -546,10 +549,9 @@ export class ApplicationRef {
546549

547550
try {
548551
this._runningTick = true;
549-
for (let view of this._views) {
550-
view.detectChanges();
551-
}
552-
this.afterRenderEffectManager.execute();
552+
553+
this.detectChangesInAttachedViews();
554+
553555
if ((typeof ngDevMode === 'undefined' || ngDevMode)) {
554556
for (let view of this._views) {
555557
view.checkNoChanges();
@@ -563,6 +565,53 @@ export class ApplicationRef {
563565
}
564566
}
565567

568+
private detectChangesInAttachedViews() {
569+
let runs = 0;
570+
do {
571+
if (runs === MAXIMUM_REFRESH_RERUNS) {
572+
throw new RuntimeError(
573+
RuntimeErrorCode.INFINITE_CHANGE_DETECTION,
574+
ngDevMode &&
575+
'Changes in afterRender or afterNextRender hooks caused infinite change detection while refresh views.');
576+
}
577+
578+
const isFirstPass = runs === 0;
579+
for (let {_lView, notifyErrorHandler} of this._views) {
580+
// When re-checking, only check views which actually need it.
581+
if (!isFirstPass && !shouldRecheckView(_lView)) {
582+
continue;
583+
}
584+
this.detectChangesInView(_lView, notifyErrorHandler, isFirstPass);
585+
}
586+
this.afterRenderEffectManager.execute();
587+
588+
runs++;
589+
} while (this._views.some(({_lView}) => shouldRecheckView(_lView)));
590+
}
591+
592+
private detectChangesInView(lView: LView, notifyErrorHandler: boolean, isFirstPass: boolean) {
593+
let mode: ChangeDetectionMode;
594+
if (isFirstPass) {
595+
// The first pass is always in Global mode, which includes `CheckAlways` views.
596+
mode = ChangeDetectionMode.Global;
597+
// Add `RefreshView` flag to ensure this view is refreshed if not already dirty.
598+
// `RefreshView` flag is used intentionally over `Dirty` because it gets cleared before
599+
// executing any of the actual refresh code while the `Dirty` flag doesn't get cleared
600+
// until the end of the refresh. Using `RefreshView` prevents creating a potential
601+
// difference in the state of the LViewFlags during template execution.
602+
lView[FLAGS] |= LViewFlags.RefreshView;
603+
} else if (lView[FLAGS] & LViewFlags.Dirty) {
604+
// The root view has been explicitly marked for check, so check it in Global mode.
605+
mode = ChangeDetectionMode.Global;
606+
} else {
607+
// The view has not been marked for check, but contains a view marked for refresh
608+
// (likely via a signal). Start this change detection in Targeted mode to skip the root
609+
// view and check just the view(s) that need refreshed.
610+
mode = ChangeDetectionMode.Targeted;
611+
}
612+
detectChangesInternal(lView, notifyErrorHandler, mode);
613+
}
614+
566615
/**
567616
* Attaches a view so that it will be dirty checked.
568617
* The view will be automatically detached when it is destroyed.
@@ -715,3 +764,8 @@ export function whenStable(applicationRef: ApplicationRef): Promise<void> {
715764

716765
return whenStablePromise;
717766
}
767+
768+
769+
function shouldRecheckView(view: LView): boolean {
770+
return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty);
771+
}

‎packages/core/src/render3/instructions/change_detection.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,10 @@ import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCo
2525
/**
2626
* The maximum number of times the change detection traversal will rerun before throwing an error.
2727
*/
28-
const MAXIMUM_REFRESH_RERUNS = 100;
28+
export const MAXIMUM_REFRESH_RERUNS = 100;
2929

30-
export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
30+
export function detectChangesInternal(
31+
lView: LView, notifyErrorHandler = true, mode = ChangeDetectionMode.Global) {
3132
const environment = lView[ENVIRONMENT];
3233
const rendererFactory = environment.rendererFactory;
3334

@@ -41,7 +42,7 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
4142
}
4243

4344
try {
44-
detectChangesInViewWhileDirty(lView);
45+
detectChangesInViewWhileDirty(lView, mode);
4546
} catch (error) {
4647
if (notifyErrorHandler) {
4748
handleError(lView, error);
@@ -58,8 +59,8 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
5859
}
5960
}
6061

61-
function detectChangesInViewWhileDirty(lView: LView) {
62-
detectChangesInView(lView, ChangeDetectionMode.Global);
62+
function detectChangesInViewWhileDirty(lView: LView, mode: ChangeDetectionMode) {
63+
detectChangesInView(lView, mode);
6364

6465
let retries = 0;
6566
// 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)
99100
* The change detection traversal algorithm switches between these modes based on various
100101
* conditions.
101102
*/
102-
const enum ChangeDetectionMode {
103+
export const enum ChangeDetectionMode {
103104
/**
104105
* In `Global` mode, `Dirty` and `CheckAlways` views are refreshed as well as views with the
105106
* `RefreshView` flag.

‎packages/core/src/render3/util/view_utils.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,8 +197,9 @@ export function walkUpViews(nestingLevel: number, currentView: LView): LView {
197197
}
198198

199199
export function requiresRefreshOrTraversal(lView: LView) {
200-
return lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) ||
201-
lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty;
200+
return !!(
201+
lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) ||
202+
lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty);
202203
}
203204

204205

‎packages/core/src/render3/view_ref.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ export class ViewRef<T> implements EmbeddedViewRef<T>, ChangeDetectorRefInterfac
5757
*
5858
* This may be different from `_lView` if the `_cdRefInjectingView` is an embedded view.
5959
*/
60-
private _cdRefInjectingView?: LView, private readonly notifyErrorHandler = true) {}
60+
private _cdRefInjectingView?: LView, readonly notifyErrorHandler = true) {}
6161

6262
get context(): T {
6363
return this._lView[CONTEXT] as unknown as T;

‎packages/core/test/acceptance/after_render_hook_spec.ts‎

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import {PLATFORM_BROWSER_ID, PLATFORM_SERVER_ID} from '@angular/common/src/platform_id';
10-
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';
10+
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';
1111
import {NoopNgZone} from '@angular/core/src/zone/ng_zone';
1212
import {TestBed} from '@angular/core/testing';
1313

@@ -894,6 +894,98 @@ describe('after render hooks', () => {
894894
]);
895895
});
896896
});
897+
898+
it('allows writing to a signal in afterRender', () => {
899+
const counter = signal(0);
900+
901+
@Component({
902+
selector: 'test-component',
903+
standalone: true,
904+
template: ` {{counter()}} `,
905+
})
906+
class TestCmp {
907+
counter = counter;
908+
injector = inject(EnvironmentInjector);
909+
ngOnInit() {
910+
afterNextRender(() => {
911+
this.counter.set(1);
912+
}, {injector: this.injector});
913+
}
914+
}
915+
TestBed.configureTestingModule(
916+
{providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]});
917+
918+
const fixture = TestBed.createComponent(TestCmp);
919+
const appRef = TestBed.inject(ApplicationRef);
920+
appRef.attachView(fixture.componentRef.hostView);
921+
appRef.tick();
922+
expect(fixture.nativeElement.innerText).toBe('1');
923+
});
924+
925+
it('allows updating state and calling markForCheck in afterRender', () => {
926+
@Component({
927+
selector: 'test-component',
928+
standalone: true,
929+
template: ` {{counter}} `,
930+
})
931+
class TestCmp {
932+
counter = 0;
933+
injector = inject(EnvironmentInjector);
934+
cdr = inject(ChangeDetectorRef);
935+
ngOnInit() {
936+
afterNextRender(() => {
937+
this.counter = 1;
938+
this.cdr.markForCheck();
939+
}, {injector: this.injector});
940+
}
941+
}
942+
TestBed.configureTestingModule(
943+
{providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]});
944+
945+
const fixture = TestBed.createComponent(TestCmp);
946+
const appRef = TestBed.inject(ApplicationRef);
947+
appRef.attachView(fixture.componentRef.hostView);
948+
appRef.tick();
949+
expect(fixture.nativeElement.innerText).toBe('1');
950+
});
951+
952+
it('throws error when causing infinite updates', () => {
953+
const counter = signal(0);
954+
955+
@Component({
956+
selector: 'test-component',
957+
standalone: true,
958+
template: ` {{counter()}} `,
959+
})
960+
class TestCmp {
961+
counter = counter;
962+
injector = inject(EnvironmentInjector);
963+
ngOnInit() {
964+
afterRender(() => {
965+
this.counter.update(v => v + 1);
966+
}, {injector: this.injector});
967+
}
968+
}
969+
TestBed.configureTestingModule({
970+
providers: [
971+
{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID},
972+
{
973+
provide: ErrorHandler, useClass: class extends ErrorHandler {
974+
override handleError(error: any) {
975+
throw error;
976+
}
977+
}
978+
},
979+
]
980+
});
981+
982+
const fixture = TestBed.createComponent(TestCmp);
983+
const appRef = TestBed.inject(ApplicationRef);
984+
appRef.attachView(fixture.componentRef.hostView);
985+
expect(() => {
986+
appRef.tick();
987+
}).toThrowError(/NG0103.*(afterRender|afterNextRender)/);
988+
});
897989
});
898990

899991
describe('server', () => {

‎packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -887,36 +887,6 @@ describe('OnPush components with signals', () => {
887887
expect(fixture.nativeElement.innerText).toEqual('2');
888888
});
889889

890-
// Note: We probably don't want this to throw but need to decide how to handle reruns
891-
// This asserts current behavior and should be updated when this is handled
892-
it('throws error when writing to a signal in afterRender', () => {
893-
const counter = signal(0);
894-
895-
@Component({
896-
selector: 'test-component',
897-
standalone: true,
898-
template: ` {{counter()}} `,
899-
})
900-
class TestCmp {
901-
counter = counter;
902-
injector = inject(EnvironmentInjector);
903-
ngOnInit() {
904-
afterNextRender(() => {
905-
this.counter.set(1);
906-
}, {injector: this.injector});
907-
}
908-
}
909-
TestBed.configureTestingModule(
910-
{providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]});
911-
912-
const fixture = TestBed.createComponent(TestCmp);
913-
const appRef = TestBed.inject(ApplicationRef);
914-
appRef.attachView(fixture.componentRef.hostView);
915-
const spy = spyOn(console, 'error');
916-
appRef.tick();
917-
expect(spy.calls.first().args[1]).toMatch(/ExpressionChanged/);
918-
});
919-
920890
it('destroys all signal consumers when destroying the view tree', () => {
921891
const val = signal(1);
922892
const double = computed(() => val() * 2);

‎packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -797,6 +797,9 @@
797797
{
798798
"name": "detectChangesInViewIfAttached"
799799
},
800+
{
801+
"name": "detectChangesInternal"
802+
},
800803
{
801804
"name": "diPublicInInjector"
802805
},
@@ -1352,6 +1355,9 @@
13521355
{
13531356
"name": "shimStylesContent"
13541357
},
1358+
{
1359+
"name": "shouldRecheckView"
1360+
},
13551361
{
13561362
"name": "shouldSearchParent"
13571363
},

‎packages/core/test/bundling/animations/bundle.golden_symbols.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -863,6 +863,9 @@
863863
{
864864
"name": "detectChangesInViewIfAttached"
865865
},
866+
{
867+
"name": "detectChangesInternal"
868+
},
866869
{
867870
"name": "diPublicInInjector"
868871
},
@@ -1424,6 +1427,9 @@
14241427
{
14251428
"name": "shimStylesContent"
14261429
},
1430+
{
1431+
"name": "shouldRecheckView"
1432+
},
14271433
{
14281434
"name": "shouldSearchParent"
14291435
},

‎packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -647,6 +647,9 @@
647647
{
648648
"name": "detectChangesInViewIfAttached"
649649
},
650+
{
651+
"name": "detectChangesInternal"
652+
},
650653
{
651654
"name": "diPublicInInjector"
652655
},
@@ -1130,6 +1133,9 @@
11301133
{
11311134
"name": "shimStylesContent"
11321135
},
1136+
{
1137+
"name": "shouldRecheckView"
1138+
},
11331139
{
11341140
"name": "shouldSearchParent"
11351141
},

‎packages/core/test/bundling/defer/bundle.golden_symbols.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -740,6 +740,9 @@
740740
{
741741
"name": "detectChangesInViewIfAttached"
742742
},
743+
{
744+
"name": "detectChangesInternal"
745+
},
743746
{
744747
"name": "diPublicInInjector"
745748
},
@@ -2249,6 +2252,9 @@
22492252
{
22502253
"name": "shimStylesContent"
22512254
},
2255+
{
2256+
"name": "shouldRecheckView"
2257+
},
22522258
{
22532259
"name": "shouldSearchParent"
22542260
},

0 commit comments

Comments
 (0)