Skip to content

Commit bdd61c7

Browse files
alxhubalxhub
committed
fix(core): replace assertion with more intentional error (angular#52234)
Issue angular#50320 shows that in some cases, updating a signal that's a dependency of a template during change detection of that template can have several adverse effects. This can happen, for example, if the signal is set during the lifecycle hook of a directive within the same template that reads the signal. This can cause a few things to happen: * Straightforwardly, it can cause `ExpressionChanged` errors. * Surprisingly, it can cause an assertion within the `ReactiveLViewConsumer` to fail. * Very surprisingly, it can cause change detection for an `OnPush` component to stop working. The root cause of these later behaviors is subtle, and is ultimately a desync between the reactive graph and the view tree's notion of "dirty" for a given view. This will be fixed with further work planned for change detection to handle such updates directly. Until then, this commit improves the DX through two changes: 1. The mechanism of "committing" `ReactiveLViewConsumer`s to a view is changed to use the `consumerOnSignalRead` hook from the reactive graph. This prevents the situation which required the assertion in the first place. 2. A `console.warn` warning is added when a view is marked dirty via a signal while it's still executing. The warning informs users that they're pushing data against the direction of change detection, risking `ExpressionChanged` or other issues. It's a warning and not an error because the check is overly broad and captures situations where the application would not actually break as a result, such as if a `computed` marked the template dirty but still returned the same value. PR Close angular#52234
1 parent 7888819 commit bdd61c7

File tree

16 files changed

+121
-153
lines changed

16 files changed

+121
-153
lines changed

‎goldens/size-tracking/aio-payloads.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
"aio-local": {
1313
"uncompressed": {
1414
"runtime": 4252,
15-
"main": 507632,
15+
"main": 512673,
1616
"polyfills": 33862,
1717
"styles": 60209,
1818
"light-theme": 34317,

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

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery';
1313
import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
1414
import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
1515
import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
16-
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView} from '../interfaces/view';
16+
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView} from '../interfaces/view';
1717
import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
1818
import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
1919
import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
@@ -159,17 +159,23 @@ export function refreshView<T>(
159159
// execute pre-order hooks (OnInit, OnChanges, DoCheck)
160160
// PERF WARNING: do NOT extract this to a separate function without running benchmarks
161161
if (!isInCheckNoChangesPass) {
162-
if (hooksInitPhaseCompleted) {
163-
const preOrderCheckHooks = tView.preOrderCheckHooks;
164-
if (preOrderCheckHooks !== null) {
165-
executeCheckHooks(lView, preOrderCheckHooks, null);
166-
}
167-
} else {
168-
const preOrderHooks = tView.preOrderHooks;
169-
if (preOrderHooks !== null) {
170-
executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null);
162+
const consumer = lView[REACTIVE_TEMPLATE_CONSUMER];
163+
try {
164+
consumer && (consumer.isRunning = true);
165+
if (hooksInitPhaseCompleted) {
166+
const preOrderCheckHooks = tView.preOrderCheckHooks;
167+
if (preOrderCheckHooks !== null) {
168+
executeCheckHooks(lView, preOrderCheckHooks, null);
169+
}
170+
} else {
171+
const preOrderHooks = tView.preOrderHooks;
172+
if (preOrderHooks !== null) {
173+
executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null);
174+
}
175+
incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun);
171176
}
172-
incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun);
177+
} finally {
178+
consumer && (consumer.isRunning = false);
173179
}
174180
}
175181

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ import {assertPureTNodeType, assertTNodeType} from '../node_assert';
4343
import {clearElementContents, updateTextNode} from '../node_manipulation';
4444
import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher';
4545
import {profiler, ProfilerEvent} from '../profiler';
46-
import {commitLViewConsumerIfHasProducers, getReactiveLViewConsumer} from '../reactive_lview_consumer';
46+
import {getReactiveLViewConsumer} from '../reactive_lview_consumer';
4747
import {getBindingsEnabled, getCurrentDirectiveIndex, getCurrentParentTNode, getCurrentTNodePlaceholderOk, getSelectedIndex, isCurrentTNodeParent, isInCheckNoChangesMode, isInI18nBlock, isInSkipHydrationBlock, setBindingRootForHostBindings, setCurrentDirectiveIndex, setCurrentQueryIndex, setCurrentTNode, setSelectedIndex} from '../state';
4848
import {NO_CHANGE} from '../tokens';
4949
import {mergeHostAttrs} from '../util/attrs_utils';
@@ -82,18 +82,17 @@ export function processHostBindingOpCodes(tView: TView, lView: LView): void {
8282
setBindingRootForHostBindings(bindingRootIndx, directiveIdx);
8383
consumer.dirty = false;
8484
const prevConsumer = consumerBeforeComputation(consumer);
85+
consumer.isRunning = true;
8586
try {
8687
const context = lView[directiveIdx];
8788
hostBindingFn(RenderFlags.Update, context);
8889
} finally {
8990
consumerAfterComputation(consumer, prevConsumer);
91+
consumer.isRunning = false;
9092
}
9193
}
9294
}
9395
} finally {
94-
if (lView[REACTIVE_HOST_BINDING_CONSUMER] === null) {
95-
commitLViewConsumerIfHasProducers(lView, REACTIVE_HOST_BINDING_CONSUMER);
96-
}
9796
setSelectedIndex(-1);
9897
}
9998
}
@@ -275,15 +274,14 @@ export function executeTemplate<T>(
275274
try {
276275
if (effectiveConsumer !== null) {
277276
effectiveConsumer.dirty = false;
277+
effectiveConsumer.isRunning = true;
278278
}
279279
templateFn(rf, context);
280280
} finally {
281281
consumerAfterComputation(effectiveConsumer, prevConsumer);
282+
effectiveConsumer && (effectiveConsumer.isRunning = false);
282283
}
283284
} finally {
284-
if (isUpdatePhase && lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
285-
commitLViewConsumerIfHasProducers(lView, REACTIVE_TEMPLATE_CONSUMER);
286-
}
287285
setSelectedIndex(prevSelectedIndex);
288286

289287
const postHookType =

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

Lines changed: 27 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,17 @@
88

99
import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals';
1010

11-
import {assertDefined, assertEqual} from '../util/assert';
11+
import {RuntimeError} from '../errors';
12+
import {assertDefined} from '../util/assert';
1213

1314
import {markViewDirty} from './instructions/mark_view_dirty';
1415
import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
1516

1617
let currentConsumer: ReactiveLViewConsumer|null = null;
1718
export interface ReactiveLViewConsumer extends ReactiveNode {
18-
lView: LView|null;
19+
lView: LView;
20+
slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER;
21+
isRunning: boolean;
1922
}
2023

2124
/**
@@ -26,50 +29,40 @@ export interface ReactiveLViewConsumer extends ReactiveNode {
2629
export function getReactiveLViewConsumer(
2730
lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER):
2831
ReactiveLViewConsumer {
29-
return lView[slot] ?? getOrCreateCurrentLViewConsumer();
32+
return lView[slot] ?? getOrCreateCurrentLViewConsumer(lView, slot);
3033
}
3134

32-
/**
33-
* Assigns the `currentTemplateContext` to its LView's `REACTIVE_CONSUMER` slot if there are tracked
34-
* producers.
35-
*
36-
* The presence of producers means that a signal was read while the consumer was the active
37-
* consumer.
38-
*
39-
* If no producers are present, we do not assign the current template context. This also means we
40-
* can just reuse the template context for the next LView.
41-
*/
42-
export function commitLViewConsumerIfHasProducers(
43-
lView: LView,
44-
slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER): void {
45-
const consumer = getOrCreateCurrentLViewConsumer();
46-
if (!consumer.producerNode?.length) {
47-
return;
48-
}
49-
50-
lView[slot] = currentConsumer;
51-
consumer.lView = lView;
52-
currentConsumer = createLViewConsumer();
53-
}
54-
55-
const REACTIVE_LVIEW_CONSUMER_NODE: ReactiveLViewConsumer = {
35+
const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'|'slot'> = {
5636
...REACTIVE_NODE,
5737
consumerIsAlwaysLive: true,
5838
consumerMarkedDirty: (node: ReactiveLViewConsumer) => {
59-
(typeof ngDevMode === 'undefined' || ngDevMode) &&
60-
assertDefined(
61-
node.lView,
62-
'Updating a signal during template or host binding execution is not allowed.');
63-
markViewDirty(node.lView!);
39+
if (ngDevMode && node.isRunning) {
40+
console.warn(
41+
`Angular detected a signal being set which makes the template for this component dirty` +
42+
` while it's being executed, which is not currently supported and will likely result` +
43+
` in ExpressionChangedAfterItHasBeenChecked errors or future updates not working` +
44+
` entirely.`);
45+
}
46+
markViewDirty(node.lView);
47+
},
48+
consumerOnSignalRead(this: ReactiveLViewConsumer): void {
49+
if (currentConsumer !== this) {
50+
return;
51+
}
52+
this.lView[this.slot] = currentConsumer;
53+
currentConsumer = null;
6454
},
65-
lView: null,
55+
isRunning: false,
6656
};
6757

6858
function createLViewConsumer(): ReactiveLViewConsumer {
6959
return Object.create(REACTIVE_LVIEW_CONSUMER_NODE);
7060
}
7161

72-
function getOrCreateCurrentLViewConsumer() {
62+
function getOrCreateCurrentLViewConsumer(
63+
lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER) {
7364
currentConsumer ??= createLViewConsumer();
65+
currentConsumer.lView = lView;
66+
currentConsumer.slot = slot;
7467
return currentConsumer;
7568
}

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

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

99
import {NgIf} from '@angular/common';
10-
import {ChangeDetectionStrategy, Component, Input, signal, ViewChild} from '@angular/core';
10+
import {ChangeDetectionStrategy, Component, Directive, Input, signal, ViewChild} from '@angular/core';
1111
import {TestBed} from '@angular/core/testing';
1212

1313
describe('CheckAlways components', () => {
@@ -370,4 +370,74 @@ describe('OnPush components with signals', () => {
370370
fixture.detectChanges();
371371
expect(fixture.nativeElement.outerHTML).not.toContain('blue');
372372
});
373+
374+
it('should warn when writing to signals during change-detecting a given template, in advance()',
375+
() => {
376+
const counter = signal(0);
377+
378+
@Directive({
379+
standalone: true,
380+
selector: '[misunderstood]',
381+
})
382+
class MisunderstoodDir {
383+
ngOnInit(): void {
384+
counter.update((c) => c + 1);
385+
}
386+
}
387+
388+
@Component({
389+
selector: 'test-component',
390+
standalone: true,
391+
imports: [MisunderstoodDir],
392+
template: `
393+
{{counter()}}<div misunderstood></div>{{ 'force advance()' }}
394+
`,
395+
})
396+
class TestCmp {
397+
counter = counter;
398+
}
399+
400+
const consoleWarnSpy = spyOn(console, 'warn').and.callThrough();
401+
402+
const fixture = TestBed.createComponent(TestCmp);
403+
fixture.detectChanges(false);
404+
expect(consoleWarnSpy)
405+
.toHaveBeenCalledWith(jasmine.stringMatching(
406+
/will likely result in ExpressionChangedAfterItHasBeenChecked/));
407+
});
408+
409+
it('should warn when writing to signals during change-detecting a given template, at the end',
410+
() => {
411+
const counter = signal(0);
412+
413+
@Directive({
414+
standalone: true,
415+
selector: '[misunderstood]',
416+
})
417+
class MisunderstoodDir {
418+
ngOnInit(): void {
419+
counter.update((c) => c + 1);
420+
}
421+
}
422+
423+
@Component({
424+
selector: 'test-component',
425+
standalone: true,
426+
imports: [MisunderstoodDir],
427+
template: `
428+
{{counter()}}<div misunderstood></div>
429+
`,
430+
})
431+
class TestCmp {
432+
counter = counter;
433+
}
434+
435+
const consoleWarnSpy = spyOn(console, 'warn').and.callThrough();
436+
437+
const fixture = TestBed.createComponent(TestCmp);
438+
fixture.detectChanges(false);
439+
expect(consoleWarnSpy)
440+
.toHaveBeenCalledWith(jasmine.stringMatching(
441+
/will likely result in ExpressionChangedAfterItHasBeenChecked/));
442+
});
373443
});

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -716,9 +716,6 @@
716716
{
717717
"name": "collectNativeNodesInLContainer"
718718
},
719-
{
720-
"name": "commitLViewConsumerIfHasProducers"
721-
},
722719
{
723720
"name": "computeStaticStyling"
724721
},
@@ -782,9 +779,6 @@
782779
{
783780
"name": "createLView"
784781
},
785-
{
786-
"name": "createLViewConsumer"
787-
},
788782
{
789783
"name": "createNodeInjector"
790784
},
@@ -971,9 +965,6 @@
971965
{
972966
"name": "getOrCreateComponentTView"
973967
},
974-
{
975-
"name": "getOrCreateCurrentLViewConsumer"
976-
},
977968
{
978969
"name": "getOrCreateInjectable"
979970
},

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -776,9 +776,6 @@
776776
{
777777
"name": "collectNativeNodesInLContainer"
778778
},
779-
{
780-
"name": "commitLViewConsumerIfHasProducers"
781-
},
782779
{
783780
"name": "computeStaticStyling"
784781
},
@@ -845,9 +842,6 @@
845842
{
846843
"name": "createLView"
847844
},
848-
{
849-
"name": "createLViewConsumer"
850-
},
851845
{
852846
"name": "createNodeInjector"
853847
},
@@ -1037,9 +1031,6 @@
10371031
{
10381032
"name": "getOrCreateComponentTView"
10391033
},
1040-
{
1041-
"name": "getOrCreateCurrentLViewConsumer"
1042-
},
10431034
{
10441035
"name": "getOrCreateInjectable"
10451036
},

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -584,9 +584,6 @@
584584
{
585585
"name": "collectNativeNodesInLContainer"
586586
},
587-
{
588-
"name": "commitLViewConsumerIfHasProducers"
589-
},
590587
{
591588
"name": "computeStaticStyling"
592589
},
@@ -635,9 +632,6 @@
635632
{
636633
"name": "createLView"
637634
},
638-
{
639-
"name": "createLViewConsumer"
640-
},
641635
{
642636
"name": "createNodeInjector"
643637
},
@@ -806,9 +800,6 @@
806800
{
807801
"name": "getOrCreateComponentTView"
808802
},
809-
{
810-
"name": "getOrCreateCurrentLViewConsumer"
811-
},
812803
{
813804
"name": "getOrCreateInjectable"
814805
},

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -662,9 +662,6 @@
662662
{
663663
"name": "collectNativeNodesInLContainer"
664664
},
665-
{
666-
"name": "commitLViewConsumerIfHasProducers"
667-
},
668665
{
669666
"name": "computeStaticStyling"
670667
},
@@ -716,9 +713,6 @@
716713
{
717714
"name": "createLView"
718715
},
719-
{
720-
"name": "createLViewConsumer"
721-
},
722716
{
723717
"name": "createNodeInjector"
724718
},
@@ -908,9 +902,6 @@
908902
{
909903
"name": "getOrCreateComponentTView"
910904
},
911-
{
912-
"name": "getOrCreateCurrentLViewConsumer"
913-
},
914905
{
915906
"name": "getOrCreateInjectable"
916907
},

0 commit comments

Comments
 (0)