From 58d74a29f8476aba2936d058394491fd7e48bf81 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 26 Oct 2023 16:40:19 -0700 Subject: [PATCH] refactor(core): Remove warning about signal set during template execution (#52476) The significance of the combination of #51854 and #52302 went mostly unnoticed. The first removed a unidirectional data flow constraint for transplanted views and the second updated the signal implementation to share transplanted view logic. The result is that we automatically get behavior that (mostly) removes `ExpressionChangedAfterItWasCheckedError` when signals are used to drive application state to DOM synchronization. fixes #50320 PR Close #52476 --- .../render3/instructions/change_detection.ts | 28 ++--- .../core/src/render3/instructions/shared.ts | 4 - .../src/render3/reactive_lview_consumer.ts | 9 -- .../change_detection_signals_in_zones_spec.ts | 114 ++++++++++++------ 4 files changed, 89 insertions(+), 66 deletions(-) diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 621653e0520bb..99d8f8ea3b306 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, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; +import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView, TViewType} 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'; @@ -160,23 +160,17 @@ 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) { - 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); + 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); } - } finally { - consumer && (consumer.isRunning = false); + incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun); } } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 5339058590c2e..c74c707c2ab96 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -82,13 +82,11 @@ 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; } } } @@ -274,12 +272,10 @@ 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 { setSelectedIndex(prevSelectedIndex); diff --git a/packages/core/src/render3/reactive_lview_consumer.ts b/packages/core/src/render3/reactive_lview_consumer.ts index 2bb276a3de632..979b6ad50841e 100644 --- a/packages/core/src/render3/reactive_lview_consumer.ts +++ b/packages/core/src/render3/reactive_lview_consumer.ts @@ -15,7 +15,6 @@ let currentConsumer: ReactiveLViewConsumer|null = null; export interface ReactiveLViewConsumer extends ReactiveNode { lView: LView; slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER; - isRunning: boolean; } /** @@ -33,13 +32,6 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit ...REACTIVE_NODE, consumerIsAlwaysLive: true, consumerMarkedDirty: (node: ReactiveLViewConsumer) => { - 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.`); - } markViewDirtyFromSignal(node.lView); }, consumerOnSignalRead(this: ReactiveLViewConsumer): void { @@ -49,7 +41,6 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit this.lView[this.slot] = currentConsumer; currentConsumer = null; }, - isRunning: false, }; function createLViewConsumer(): ReactiveLViewConsumer { 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 6fe555c4e8e1c..1303ba76c2d0d 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 {NgFor, NgIf} from '@angular/common'; -import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Input, signal, untracked, ViewChild} from '@angular/core'; +import {ChangeDetectionStrategy,inject, ChangeDetectorRef, Component, Directive, Input, signal, untracked, ViewChild} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('CheckAlways components', () => { @@ -483,7 +483,7 @@ describe('OnPush components with signals', () => { expect(fixture.nativeElement.outerHTML).not.toContain('blue'); }); - it('should warn when writing to signals during change-detecting a given template, in advance()', + it('should be able to write to signals during change-detecting a given template, in advance()', () => { const counter = signal(0); @@ -509,49 +509,63 @@ describe('OnPush components with signals', () => { 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/)); + // CheckNoChanges should not throw ExpressionChanged error + // and signal value is updated to latest value with 1 `detectChanges` + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('1'); + expect(fixture.nativeElement.innerText).toContain('force advance()'); }); - it('should warn when writing to signals during change-detecting a given template, at the end', - () => { - const counter = signal(0); + it('should allow 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); - } - } + @Directive({ + standalone: true, + selector: '[misunderstood]', + }) + class MisunderstoodDir { + ngOnInit(): void { + counter.update((c) => c + 1); + } + } - @Component({ - selector: 'test-component', - standalone: true, - imports: [MisunderstoodDir], - template: ` + @Component({ + selector: 'test-component', + standalone: true, + imports: [MisunderstoodDir], + template: ` {{counter()}}
`, - }) - class TestCmp { - counter = counter; - } + }) + class TestCmp { + counter = counter; + } - const consoleWarnSpy = spyOn(console, 'warn').and.callThrough(); + const fixture = TestBed.createComponent(TestCmp); + // CheckNoChanges should not throw ExpressionChanged error + // and signal value is updated to latest value with 1 `detectChanges` + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toBe('1'); + }); - const fixture = TestBed.createComponent(TestCmp); - fixture.detectChanges(false); - expect(consoleWarnSpy) - .toHaveBeenCalledWith(jasmine.stringMatching( - /will likely result in ExpressionChangedAfterItHasBeenChecked/)); - }); + it('should allow writing to signals in afterViewInit', () => { + @Component({ + template: '{{loading()}}', + standalone: true, + }) + class MyComp { + loading = signal(true); + // Classic example of what would have caused ExpressionChanged...Error + ngAfterViewInit() { + this.loading.set(false); + } + } + + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toBe('false'); + }); it('does not refresh view if signal marked dirty but did not change', () => { const val = signal('initial', {equal: () => true}); @@ -749,6 +763,34 @@ describe('OnPush components with signals', () => { expect(fixture.componentInstance.signalChild.afterViewCheckedRuns).toBe(1); }); }); + + it('can refresh the root of change detection if updated after checked', () => { + const val = signal(1); + @Component({ + template: '', + selector: 'child', + standalone: true, + }) + class Child { + ngOnInit() { + val.set(2); + } + } + + @Component({ + template: '{{val()}}', + imports: [Child], + standalone: true, + }) + class SignalComponent { + val = val; + cdr = inject(ChangeDetectorRef); + } + + const fixture = TestBed.createComponent(SignalComponent); + fixture.componentInstance.cdr.detectChanges(); + expect(fixture.nativeElement.innerText).toEqual('2'); + }); });