diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 8fe630464d2ded..5e292d8c547705 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks'; import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container'; import {ComponentTemplate, RenderFlags} from '../interfaces/definition'; -import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView} from '../interfaces/view'; +import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView} from '../interfaces/view'; import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state'; import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils'; import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils'; @@ -159,17 +159,23 @@ export function refreshView( // execute pre-order hooks (OnInit, OnChanges, DoCheck) // PERF WARNING: do NOT extract this to a separate function without running benchmarks if (!isInCheckNoChangesPass) { - if (hooksInitPhaseCompleted) { - const preOrderCheckHooks = tView.preOrderCheckHooks; - if (preOrderCheckHooks !== null) { - executeCheckHooks(lView, preOrderCheckHooks, null); - } - } else { - const preOrderHooks = tView.preOrderHooks; - if (preOrderHooks !== null) { - executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null); + const consumer = lView[REACTIVE_TEMPLATE_CONSUMER]; + try { + consumer && (consumer.isRunning = true); + if (hooksInitPhaseCompleted) { + const preOrderCheckHooks = tView.preOrderCheckHooks; + if (preOrderCheckHooks !== null) { + executeCheckHooks(lView, preOrderCheckHooks, null); + } + } else { + const preOrderHooks = tView.preOrderHooks; + if (preOrderHooks !== null) { + executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null); + } + incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun); } - incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun); + } finally { + consumer && (consumer.isRunning = false); } } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 7b86be4bb5e148..5339058590c2e0 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -43,7 +43,7 @@ import {assertPureTNodeType, assertTNodeType} from '../node_assert'; import {clearElementContents, updateTextNode} from '../node_manipulation'; import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher'; import {profiler, ProfilerEvent} from '../profiler'; -import {commitLViewConsumerIfHasProducers, getReactiveLViewConsumer} from '../reactive_lview_consumer'; +import {getReactiveLViewConsumer} from '../reactive_lview_consumer'; import {getBindingsEnabled, getCurrentDirectiveIndex, getCurrentParentTNode, getCurrentTNodePlaceholderOk, getSelectedIndex, isCurrentTNodeParent, isInCheckNoChangesMode, isInI18nBlock, isInSkipHydrationBlock, setBindingRootForHostBindings, setCurrentDirectiveIndex, setCurrentQueryIndex, setCurrentTNode, setSelectedIndex} from '../state'; import {NO_CHANGE} from '../tokens'; import {mergeHostAttrs} from '../util/attrs_utils'; @@ -82,18 +82,17 @@ export function processHostBindingOpCodes(tView: TView, lView: LView): void { setBindingRootForHostBindings(bindingRootIndx, directiveIdx); consumer.dirty = false; const prevConsumer = consumerBeforeComputation(consumer); + consumer.isRunning = true; try { const context = lView[directiveIdx]; hostBindingFn(RenderFlags.Update, context); } finally { consumerAfterComputation(consumer, prevConsumer); + consumer.isRunning = false; } } } } finally { - if (lView[REACTIVE_HOST_BINDING_CONSUMER] === null) { - commitLViewConsumerIfHasProducers(lView, REACTIVE_HOST_BINDING_CONSUMER); - } setSelectedIndex(-1); } } @@ -275,15 +274,14 @@ export function executeTemplate( try { if (effectiveConsumer !== null) { effectiveConsumer.dirty = false; + effectiveConsumer.isRunning = true; } templateFn(rf, context); } finally { consumerAfterComputation(effectiveConsumer, prevConsumer); + effectiveConsumer && (effectiveConsumer.isRunning = false); } } finally { - if (isUpdatePhase && lView[REACTIVE_TEMPLATE_CONSUMER] === null) { - commitLViewConsumerIfHasProducers(lView, REACTIVE_TEMPLATE_CONSUMER); - } setSelectedIndex(prevSelectedIndex); const postHookType = diff --git a/packages/core/src/render3/reactive_lview_consumer.ts b/packages/core/src/render3/reactive_lview_consumer.ts index 7c6eedf1f6dade..ed7cdedc15aa2d 100644 --- a/packages/core/src/render3/reactive_lview_consumer.ts +++ b/packages/core/src/render3/reactive_lview_consumer.ts @@ -8,14 +8,17 @@ import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals'; -import {assertDefined, assertEqual} from '../util/assert'; +import {RuntimeError} from '../errors'; +import {assertDefined} from '../util/assert'; import {markViewDirty} from './instructions/mark_view_dirty'; import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view'; let currentConsumer: ReactiveLViewConsumer|null = null; export interface ReactiveLViewConsumer extends ReactiveNode { - lView: LView|null; + lView: LView; + slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER; + isRunning: boolean; } /** @@ -26,50 +29,40 @@ export interface ReactiveLViewConsumer extends ReactiveNode { export function getReactiveLViewConsumer( lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER): ReactiveLViewConsumer { - return lView[slot] ?? getOrCreateCurrentLViewConsumer(); + return lView[slot] ?? getOrCreateCurrentLViewConsumer(lView, slot); } -/** - * Assigns the `currentTemplateContext` to its LView's `REACTIVE_CONSUMER` slot if there are tracked - * producers. - * - * The presence of producers means that a signal was read while the consumer was the active - * consumer. - * - * If no producers are present, we do not assign the current template context. This also means we - * can just reuse the template context for the next LView. - */ -export function commitLViewConsumerIfHasProducers( - lView: LView, - slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER): void { - const consumer = getOrCreateCurrentLViewConsumer(); - if (!consumer.producerNode?.length) { - return; - } - - lView[slot] = currentConsumer; - consumer.lView = lView; - currentConsumer = createLViewConsumer(); -} - -const REACTIVE_LVIEW_CONSUMER_NODE: ReactiveLViewConsumer = { +const REACTIVE_LVIEW_CONSUMER_NODE: Omit = { ...REACTIVE_NODE, consumerIsAlwaysLive: true, consumerMarkedDirty: (node: ReactiveLViewConsumer) => { - (typeof ngDevMode === 'undefined' || ngDevMode) && - assertDefined( - node.lView, - 'Updating a signal during template or host binding execution is not allowed.'); - markViewDirty(node.lView!); + if (ngDevMode && node.isRunning) { + console.warn( + `Angular detected a signal being set which makes the template for this component dirty` + + ` while it's being executed, which is not currently supported and will likely result` + + ` in ExpressionChangedAfterItHasBeenChecked errors or future updates not working` + + ` entirely.`); + } + markViewDirty(node.lView); + }, + consumerOnSignalRead(this: ReactiveLViewConsumer): void { + if (currentConsumer !== this) { + return; + } + this.lView[this.slot] = currentConsumer; + currentConsumer = null; }, - lView: null, + isRunning: false, }; function createLViewConsumer(): ReactiveLViewConsumer { return Object.create(REACTIVE_LVIEW_CONSUMER_NODE); } -function getOrCreateCurrentLViewConsumer() { +function getOrCreateCurrentLViewConsumer( + lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER) { currentConsumer ??= createLViewConsumer(); + currentConsumer.lView = lView; + currentConsumer.slot = slot; return currentConsumer; } 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 6a6af4ddbe853c..7595ee41107ff5 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 @@ -7,7 +7,7 @@ */ import {NgIf} from '@angular/common'; -import {ChangeDetectionStrategy, Component, Input, signal, ViewChild} from '@angular/core'; +import {ChangeDetectionStrategy, Component, Directive, Input, signal, ViewChild} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('CheckAlways components', () => { @@ -370,4 +370,74 @@ describe('OnPush components with signals', () => { fixture.detectChanges(); expect(fixture.nativeElement.outerHTML).not.toContain('blue'); }); + + it('should warn when writing to signals during change-detecting a given template, in advance()', + () => { + const counter = signal(0); + + @Directive({ + standalone: true, + selector: '[misunderstood]', + }) + class MisunderstoodDir { + ngOnInit(): void { + counter.update((c) => c + 1); + } + } + + @Component({ + selector: 'test-component', + standalone: true, + imports: [MisunderstoodDir], + template: ` + {{counter()}}
{{ 'force advance()' }} + `, + }) + class TestCmp { + counter = counter; + } + + const consoleWarnSpy = spyOn(console, 'warn').and.callThrough(); + + const fixture = TestBed.createComponent(TestCmp); + fixture.detectChanges(false); + expect(consoleWarnSpy) + .toHaveBeenCalledWith(jasmine.stringMatching( + /will likely result in ExpressionChangedAfterItHasBeenChecked/)); + }); + + it('should warn when writing to signals during change-detecting a given template, at the end', + () => { + const counter = signal(0); + + @Directive({ + standalone: true, + selector: '[misunderstood]', + }) + class MisunderstoodDir { + ngOnInit(): void { + counter.update((c) => c + 1); + } + } + + @Component({ + selector: 'test-component', + standalone: true, + imports: [MisunderstoodDir], + template: ` + {{counter()}}
+ `, + }) + class TestCmp { + counter = counter; + } + + const consoleWarnSpy = spyOn(console, 'warn').and.callThrough(); + + const fixture = TestBed.createComponent(TestCmp); + fixture.detectChanges(false); + expect(consoleWarnSpy) + .toHaveBeenCalledWith(jasmine.stringMatching( + /will likely result in ExpressionChangedAfterItHasBeenChecked/)); + }); }); 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 ea6b233c60361f..47e8a474c49baa 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -713,9 +713,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "computeStaticStyling" }, @@ -779,9 +776,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -968,9 +962,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index 61808edfcb2da1..2d1693fbc77deb 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -773,9 +773,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "computeStaticStyling" }, @@ -842,9 +839,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -1034,9 +1028,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, 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 d55e8c90e0a19f..40a252cfb8d6a6 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -584,9 +584,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "computeStaticStyling" }, @@ -635,9 +632,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -806,9 +800,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index 160a4be0132ed1..7953c9c8a73044 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -662,9 +662,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "computeStaticStyling" }, @@ -716,9 +713,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -908,9 +902,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, 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 d3f4a74e101b75..7ede144dc40ff1 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -794,9 +794,6 @@ { "name": "collectStylingFromTAttrs" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "compose" }, @@ -869,9 +866,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -1091,9 +1085,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, 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 9d05d577b4c823..52c2528d3d1f14 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 @@ -773,9 +773,6 @@ { "name": "collectStylingFromTAttrs" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "composeAsyncValidators" }, @@ -839,9 +836,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -1052,9 +1046,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, 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 12b3954c8c1103..1581f1b704a7a3 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -449,9 +449,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "concatStringsWithSpace" }, @@ -494,9 +491,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -638,9 +632,6 @@ { "name": "getNullInjector" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index 1194a59227926d..5152f0da9bb45f 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -662,9 +662,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "concatStringsWithSpace" }, @@ -707,9 +704,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -884,9 +878,6 @@ { "name": "getNullInjector" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index b0b773b755933f..95f734a4b14b9b 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -998,9 +998,6 @@ { "name": "combineLatest" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "compare" }, @@ -1082,9 +1079,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNewSegmentChildren" }, @@ -1397,9 +1391,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, 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 e8f7ca13fdccc4..7d8287a8ef77fd 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -524,9 +524,6 @@ { "name": "collectNativeNodesInLContainer" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "concatStringsWithSpace" }, @@ -566,9 +563,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -719,9 +713,6 @@ { "name": "getNullInjector" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index d772b7183dc863..6e88fdcd4208a5 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -698,9 +698,6 @@ { "name": "collectStylingFromTAttrs" }, - { - "name": "commitLViewConsumerIfHasProducers" - }, { "name": "computeStaticStyling" }, @@ -755,9 +752,6 @@ { "name": "createLView" }, - { - "name": "createLViewConsumer" - }, { "name": "createNodeInjector" }, @@ -953,9 +947,6 @@ { "name": "getOrCreateComponentTView" }, - { - "name": "getOrCreateCurrentLViewConsumer" - }, { "name": "getOrCreateInjectable" },