Skip to content

Commit

Permalink
refactor(core): Remove warning about signal set during template execu…
Browse files Browse the repository at this point in the history 
…tion

The significance of the combination of angular#51854 and angular#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 angular#50320
  • Loading branch information
@atscott
atscott committed Oct 31, 2023
1 parent c5980d6 commit 92db061
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 66 deletions.
28 changes: 11 additions & 17 deletions packages/core/src/render3/instructions/change_detection.ts
View file
Expand Up @@ -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, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
import {CONTEXT, 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';
Expand Down Expand Up @@ -159,23 +159,17 @@ export function refreshView<T>(
// 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);
}
}

Expand Down
4 changes: 0 additions & 4 deletions packages/core/src/render3/instructions/shared.ts
View file
Expand Up @@ -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;
}
}
}
Expand Down Expand Up @@ -274,12 +272,10 @@ export function executeTemplate<T>(
try {
if (effectiveConsumer !== null) {
effectiveConsumer.dirty = false;
effectiveConsumer.isRunning = true;
}
templateFn(rf, context);
} finally {
consumerAfterComputation(effectiveConsumer, prevConsumer);
effectiveConsumer && (effectiveConsumer.isRunning = false);
}
} finally {
setSelectedIndex(prevSelectedIndex);
Expand Down
9 changes: 0 additions & 9 deletions packages/core/src/render3/reactive_lview_consumer.ts
View file
Expand Up @@ -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;
}

/**
Expand All @@ -33,13 +32,6 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'|'slot'>
...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 {
Expand All @@ -49,7 +41,6 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'|'slot'>
this.lView[this.slot] = currentConsumer;
currentConsumer = null;
},
isRunning: false,
};

function createLViewConsumer(): ReactiveLViewConsumer {
Expand Down
113 changes: 77 additions & 36 deletions packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
View file
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);

Expand All @@ -509,49 +509,62 @@ 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).toBe('1force 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()}}<div misunderstood></div>
`,
})
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});
Expand Down Expand Up @@ -749,6 +762,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()}}<child />',
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');
});
});


Expand Down

0 comments on commit 92db061

Please sign in to comment.