Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): replace assertion with more intentional error #52234

Closed
wants to merge 1 commit into from
Closed

fix(core): replace assertion with more intentional error #52234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion goldens/size-tracking/aio-payloads.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"aio-local": {
"uncompressed": {
"runtime": 4252,
"main": 507632,
"main": 512673,
"polyfills": 33862,
"styles": 60209,
"light-theme": 34317,
Expand Down
28 changes: 17 additions & 11 deletions packages/core/src/render3/instructions/change_detection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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, 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';
Expand Down Expand Up @@ -159,17 +159,23 @@ 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) {
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);
}
}

Expand Down
12 changes: 5 additions & 7 deletions packages/core/src/render3/instructions/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -275,15 +274,14 @@ 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 {
if (isUpdatePhase && lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
commitLViewConsumerIfHasProducers(lView, REACTIVE_TEMPLATE_CONSUMER);
}
setSelectedIndex(prevSelectedIndex);

const postHookType =
Expand Down
61 changes: 27 additions & 34 deletions packages/core/src/render3/reactive_lview_consumer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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<ReactiveLViewConsumer, 'lView'|'slot'> = {
...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(
Copy link
Member

@JeanMeche JeanMeche Oct 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want to create a Error code for this warning ? (and use formatRuntimeError)

This would allow us to have a dedicated error page for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was our initial intention but we've decided against it. There are cases where we can't be sure if this is an actual error (ex. signals propagating dirty state without actually updating any values rendered in a template).

Having a warn for now sounds like a good balance between correctness and providing developers with useful information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with you.
My point is that we already have warnings with codes, they log a warning but do not throw. (for example https://next.angular.io/errors/NG0506 or https://next.angular.io/errors/NG0912). This could be one too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is a good point, we should definitively do that!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so worried about it as we don't expect this console.warn to live long (it might not even make it until v17 proper).

`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;
}
72 changes: 71 additions & 1 deletion packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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()}}<div misunderstood></div>{{ '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()}}<div misunderstood></div>
`,
})
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/));
});
});
9 changes: 0 additions & 9 deletions packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,9 +716,6 @@
{
"name": "collectNativeNodesInLContainer"
},
{
"name": "commitLViewConsumerIfHasProducers"
},
{
"name": "computeStaticStyling"
},
Expand Down Expand Up @@ -782,9 +779,6 @@
{
"name": "createLView"
},
{
"name": "createLViewConsumer"
},
{
"name": "createNodeInjector"
},
Expand Down Expand Up @@ -971,9 +965,6 @@
{
"name": "getOrCreateComponentTView"
},
{
"name": "getOrCreateCurrentLViewConsumer"
},
{
"name": "getOrCreateInjectable"
},
Expand Down
9 changes: 0 additions & 9 deletions packages/core/test/bundling/animations/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,9 +776,6 @@
{
"name": "collectNativeNodesInLContainer"
},
{
"name": "commitLViewConsumerIfHasProducers"
},
{
"name": "computeStaticStyling"
},
Expand Down Expand Up @@ -845,9 +842,6 @@
{
"name": "createLView"
},
{
"name": "createLViewConsumer"
},
{
"name": "createNodeInjector"
},
Expand Down Expand Up @@ -1037,9 +1031,6 @@
{
"name": "getOrCreateComponentTView"
},
{
"name": "getOrCreateCurrentLViewConsumer"
},
{
"name": "getOrCreateInjectable"
},
Expand Down
9 changes: 0 additions & 9 deletions packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,9 +584,6 @@
{
"name": "collectNativeNodesInLContainer"
},
{
"name": "commitLViewConsumerIfHasProducers"
},
{
"name": "computeStaticStyling"
},
Expand Down Expand Up @@ -635,9 +632,6 @@
{
"name": "createLView"
},
{
"name": "createLViewConsumer"
},
{
"name": "createNodeInjector"
},
Expand Down Expand Up @@ -806,9 +800,6 @@
{
"name": "getOrCreateComponentTView"
},
{
"name": "getOrCreateCurrentLViewConsumer"
},
{
"name": "getOrCreateInjectable"
},
Expand Down
9 changes: 0 additions & 9 deletions packages/core/test/bundling/defer/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,9 +662,6 @@
{
"name": "collectNativeNodesInLContainer"
},
{
"name": "commitLViewConsumerIfHasProducers"
},
{
"name": "computeStaticStyling"
},
Expand Down Expand Up @@ -716,9 +713,6 @@
{
"name": "createLView"
},
{
"name": "createLViewConsumer"
},
{
"name": "createNodeInjector"
},
Expand Down Expand Up @@ -908,9 +902,6 @@
{
"name": "getOrCreateComponentTView"
},
{
"name": "getOrCreateCurrentLViewConsumer"
},
{
"name": "getOrCreateInjectable"
},
Expand Down