Skip to content

Commit 625ca3e

Browse files
atscottAndrewKushnir
atscott
authored and
AndrewKushnir
committed
fix(core): signals should be tracked when embeddedViewRef.detectChanges is called (#55719)
This commit fixes an issue where signals in embedded views are not tracked if they are refreshed with `EmbeddedViewRef.detectChanges` directly. We had previously assumed that embedded views were always refreshed along with their hosts. PR Close #55719
1 parent 65bf5ec commit 625ca3e

File tree

14 files changed

+326
-34
lines changed

14 files changed

+326
-34
lines changed

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

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
import {
1010
consumerAfterComputation,
1111
consumerBeforeComputation,
12+
consumerDestroy,
1213
consumerPollProducersForChange,
14+
getActiveConsumer,
1315
ReactiveNode,
1416
} from '@angular/core/primitives/signals';
1517

@@ -29,12 +31,13 @@ import {
2931
REACTIVE_TEMPLATE_CONSUMER,
3032
TVIEW,
3133
TView,
32-
TViewType,
3334
} from '../interfaces/view';
3435
import {
36+
getOrCreateTemporaryConsumer,
3537
getOrBorrowReactiveLViewConsumer,
3638
maybeReturnReactiveLViewConsumer,
3739
ReactiveLViewConsumer,
40+
viewShouldHaveReactiveConsumer,
3841
} from '../reactive_lview_consumer';
3942
import {
4043
CheckNoChangesMode,
@@ -207,11 +210,27 @@ export function refreshView<T>(
207210
// - We might already be in a reactive context if this is an embedded view of the host.
208211
// - We might be descending into a view that needs a consumer.
209212
enterView(lView);
213+
let returnConsumerToPool = true;
210214
let prevConsumer: ReactiveNode | null = null;
211215
let currentConsumer: ReactiveLViewConsumer | null = null;
212-
if (!isInCheckNoChangesPass && viewShouldHaveReactiveConsumer(tView)) {
213-
currentConsumer = getOrBorrowReactiveLViewConsumer(lView);
214-
prevConsumer = consumerBeforeComputation(currentConsumer);
216+
if (!isInCheckNoChangesPass) {
217+
if (viewShouldHaveReactiveConsumer(tView)) {
218+
currentConsumer = getOrBorrowReactiveLViewConsumer(lView);
219+
prevConsumer = consumerBeforeComputation(currentConsumer);
220+
} else if (getActiveConsumer() === null) {
221+
// If the current view should not have a reactive consumer but we don't have an active consumer,
222+
// we still need to create a temporary consumer to track any signal reads in this template.
223+
// This is a rare case that can happen with `viewContainerRef.createEmbeddedView(...).detectChanges()`.
224+
// This temporary consumer marks the first parent that _should_ have a consumer for refresh.
225+
// Once that refresh happens, the signals will be tracked in the parent consumer and we can destroy
226+
// the temporary one.
227+
returnConsumerToPool = false;
228+
currentConsumer = getOrCreateTemporaryConsumer(lView);
229+
prevConsumer = consumerBeforeComputation(currentConsumer);
230+
} else if (lView[REACTIVE_TEMPLATE_CONSUMER]) {
231+
consumerDestroy(lView[REACTIVE_TEMPLATE_CONSUMER]);
232+
lView[REACTIVE_TEMPLATE_CONSUMER] = null;
233+
}
215234
}
216235

217236
try {
@@ -351,30 +370,14 @@ export function refreshView<T>(
351370
} finally {
352371
if (currentConsumer !== null) {
353372
consumerAfterComputation(currentConsumer, prevConsumer);
354-
maybeReturnReactiveLViewConsumer(currentConsumer);
373+
if (returnConsumerToPool) {
374+
maybeReturnReactiveLViewConsumer(currentConsumer);
375+
}
355376
}
356377
leaveView();
357378
}
358379
}
359380

360-
/**
361-
* Indicates if the view should get its own reactive consumer node.
362-
*
363-
* In the current design, all embedded views share a consumer with the component view. This allows
364-
* us to refresh at the component level rather than at a per-view level. In addition, root views get
365-
* their own reactive node because root component will have a host view that executes the
366-
* component's host bindings. This needs to be tracked in a consumer as well.
367-
*
368-
* To get a more granular change detection than per-component, all we would just need to update the
369-
* condition here so that a given view gets a reactive consumer which can become dirty independently
370-
* from its parent component. For example embedded views for signal components could be created with
371-
* a new type "SignalEmbeddedView" and the condition here wouldn't even need updating in order to
372-
* get granular per-view change detection for signal components.
373-
*/
374-
function viewShouldHaveReactiveConsumer(tView: TView) {
375-
return tView.type !== TViewType.Embedded;
376-
}
377-
378381
/**
379382
* Goes over embedded views (ones created through ViewContainerRef APIs) and refreshes
380383
* them by executing an associated template function.

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

Lines changed: 70 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,20 @@
88

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

11-
import {LView, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
12-
import {markAncestorsForTraversal} from './util/view_utils';
11+
import {
12+
LView,
13+
PARENT,
14+
REACTIVE_TEMPLATE_CONSUMER,
15+
TVIEW,
16+
TView,
17+
TViewType,
18+
} from './interfaces/view';
19+
import {getLViewParent, markAncestorsForTraversal, markViewForRefresh} from './util/view_utils';
20+
import {assertDefined} from '../util/assert';
1321

14-
let freeConsumers: ReactiveLViewConsumer[] = [];
22+
let freeConsumers: ReactiveNode[] = [];
1523
export interface ReactiveLViewConsumer extends ReactiveNode {
1624
lView: LView | null;
17-
slot: typeof REACTIVE_TEMPLATE_CONSUMER;
1825
}
1926

2027
/**
@@ -27,8 +34,7 @@ export function getOrBorrowReactiveLViewConsumer(lView: LView): ReactiveLViewCon
2734
}
2835

2936
function borrowReactiveLViewConsumer(lView: LView): ReactiveLViewConsumer {
30-
const consumer: ReactiveLViewConsumer =
31-
freeConsumers.pop() ?? Object.create(REACTIVE_LVIEW_CONSUMER_NODE);
37+
const consumer = freeConsumers.pop() ?? Object.create(REACTIVE_LVIEW_CONSUMER_NODE);
3238
consumer.lView = lView;
3339
return consumer;
3440
}
@@ -42,7 +48,7 @@ export function maybeReturnReactiveLViewConsumer(consumer: ReactiveLViewConsumer
4248
freeConsumers.push(consumer);
4349
}
4450

45-
const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView' | 'slot'> = {
51+
const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'> = {
4652
...REACTIVE_NODE,
4753
consumerIsAlwaysLive: true,
4854
consumerMarkedDirty: (node: ReactiveLViewConsumer) => {
@@ -52,3 +58,60 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView' | 'slot'
5258
this.lView![REACTIVE_TEMPLATE_CONSUMER] = this;
5359
},
5460
};
61+
62+
/**
63+
* Creates a temporary consumer for use with `LView`s that should not have consumers.
64+
* If the LView already has a consumer, returns the existing one instead.
65+
*
66+
* This is necessary because some APIs may cause change detection directly on an LView
67+
* that we do not want to have a consumer (Embedded views today). As a result, there
68+
* would be no active consumer from running change detection on its host component
69+
* and any signals in the LView template would be untracked. Instead, we create
70+
* this temporary consumer that marks the first parent that _should_ have a consumer
71+
* for refresh. Once change detection runs as part of that refresh, we throw away
72+
* this consumer because its signals will then be tracked by the parent's consumer.
73+
*/
74+
export function getOrCreateTemporaryConsumer(lView: LView): ReactiveLViewConsumer {
75+
const consumer = lView[REACTIVE_TEMPLATE_CONSUMER] ?? Object.create(TEMPORARY_CONSUMER_NODE);
76+
consumer.lView = lView;
77+
return consumer;
78+
}
79+
80+
const TEMPORARY_CONSUMER_NODE = {
81+
...REACTIVE_NODE,
82+
consumerIsAlwaysLive: true,
83+
consumerMarkedDirty: (node: ReactiveLViewConsumer) => {
84+
let parent = getLViewParent(node.lView!);
85+
while (parent && !viewShouldHaveReactiveConsumer(parent[TVIEW])) {
86+
parent = getLViewParent(parent);
87+
}
88+
if (!parent) {
89+
// If we can't find an appropriate parent that should have a consumer, we
90+
// don't have a way of appropriately refreshing this LView as part of application synchronization.
91+
return;
92+
}
93+
94+
markViewForRefresh(parent);
95+
},
96+
consumerOnSignalRead(this: ReactiveLViewConsumer): void {
97+
this.lView![REACTIVE_TEMPLATE_CONSUMER] = this;
98+
},
99+
};
100+
101+
/**
102+
* Indicates if the view should get its own reactive consumer node.
103+
*
104+
* In the current design, all embedded views share a consumer with the component view. This allows
105+
* us to refresh at the component level rather than at a per-view level. In addition, root views get
106+
* their own reactive node because root component will have a host view that executes the
107+
* component's host bindings. This needs to be tracked in a consumer as well.
108+
*
109+
* To get a more granular change detection than per-component, all we would just need to update the
110+
* condition here so that a given view gets a reactive consumer which can become dirty independently
111+
* from its parent component. For example embedded views for signal components could be created with
112+
* a new type "SignalEmbeddedView" and the condition here wouldn't even need updating in order to
113+
* get granular per-view change detection for signal components.
114+
*/
115+
export function viewShouldHaveReactiveConsumer(tView: TView) {
116+
return tView.type !== TViewType.Embedded;
117+
}

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

Lines changed: 68 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,16 @@
77
*/
88

99
import {NgFor, NgIf} from '@angular/common';
10-
import {PLATFORM_BROWSER_ID} from '@angular/common/src/platform_id';
1110
import {
12-
afterNextRender,
1311
ApplicationRef,
1412
ChangeDetectionStrategy,
1513
ChangeDetectorRef,
1614
Component,
1715
computed,
1816
Directive,
19-
EnvironmentInjector,
17+
ElementRef,
2018
inject,
2119
Input,
22-
PLATFORM_ID,
2320
signal,
2421
TemplateRef,
2522
ViewChild,
@@ -803,6 +800,73 @@ describe('OnPush components with signals', () => {
803800
fixture.detectChanges();
804801
expect(trim(fixture.nativeElement.textContent)).toEqual('new');
805802
});
803+
804+
it('tracks signal updates if embedded view is change detected directly', () => {
805+
@Component({
806+
changeDetection: ChangeDetectionStrategy.OnPush,
807+
template: `
808+
<ng-template #template>{{value()}}</ng-template>
809+
`,
810+
standalone: true,
811+
})
812+
class Test {
813+
value = signal('initial');
814+
@ViewChild('template', {static: true, read: TemplateRef})
815+
template!: TemplateRef<{}>;
816+
@ViewChild('template', {static: true, read: ViewContainerRef})
817+
vcr!: ViewContainerRef;
818+
}
819+
820+
const fixture = TestBed.createComponent(Test);
821+
const appRef = TestBed.inject(ApplicationRef);
822+
appRef.attachView(fixture.componentRef.hostView);
823+
appRef.tick();
824+
825+
const viewRef = fixture.componentInstance.vcr.createEmbeddedView(
826+
fixture.componentInstance.template,
827+
);
828+
viewRef.detectChanges();
829+
expect(fixture.nativeElement.innerText).toContain('initial');
830+
831+
fixture.componentInstance.value.set('new');
832+
appRef.tick();
833+
expect(fixture.nativeElement.innerText).toContain('new');
834+
});
835+
836+
it('tracks signal updates if embedded view is change detected directly before attaching', () => {
837+
@Component({
838+
changeDetection: ChangeDetectionStrategy.OnPush,
839+
template: `
840+
<ng-template #template>{{value()}}</ng-template>
841+
`,
842+
standalone: true,
843+
})
844+
class Test {
845+
value = signal('initial');
846+
@ViewChild('template', {static: true, read: TemplateRef})
847+
template!: TemplateRef<{}>;
848+
@ViewChild('template', {static: true, read: ViewContainerRef})
849+
vcr!: ViewContainerRef;
850+
element = inject(ElementRef);
851+
}
852+
853+
const fixture = TestBed.createComponent(Test);
854+
const appRef = TestBed.inject(ApplicationRef);
855+
appRef.attachView(fixture.componentRef.hostView);
856+
appRef.tick();
857+
858+
const viewRef = fixture.componentInstance.template.createEmbeddedView(
859+
fixture.componentInstance.template,
860+
);
861+
fixture.componentInstance.element.nativeElement.appendChild(viewRef.rootNodes[0]);
862+
viewRef.detectChanges();
863+
expect(fixture.nativeElement.innerText).toContain('initial');
864+
fixture.componentInstance.vcr.insert(viewRef);
865+
866+
fixture.componentInstance.value.set('new');
867+
appRef.tick();
868+
expect(fixture.nativeElement.innerText).toContain('new');
869+
});
806870
});
807871

808872
describe('shielded by non-dirty OnPush', () => {

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,9 @@
434434
{
435435
"name": "REACTIVE_LVIEW_CONSUMER_NODE"
436436
},
437+
{
438+
"name": "REACTIVE_NODE"
439+
},
437440
{
438441
"name": "REMOVE_STYLES_ON_COMPONENT_DESTROY"
439442
},
@@ -491,6 +494,9 @@
491494
{
492495
"name": "Subscription"
493496
},
497+
{
498+
"name": "TEMPORARY_CONSUMER_NODE"
499+
},
494500
{
495501
"name": "TESTABILITY"
496502
},
@@ -746,6 +752,12 @@
746752
{
747753
"name": "configureViewWithDirective"
748754
},
755+
{
756+
"name": "consumerBeforeComputation"
757+
},
758+
{
759+
"name": "consumerDestroy"
760+
},
749761
{
750762
"name": "consumerIsLive"
751763
},
@@ -1454,6 +1466,9 @@
14541466
{
14551467
"name": "viewAttachedToChangeDetector"
14561468
},
1469+
{
1470+
"name": "viewShouldHaveReactiveConsumer"
1471+
},
14571472
{
14581473
"name": "visitDslNode"
14591474
},

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,9 @@
473473
{
474474
"name": "REACTIVE_LVIEW_CONSUMER_NODE"
475475
},
476+
{
477+
"name": "REACTIVE_NODE"
478+
},
476479
{
477480
"name": "REMOVE_STYLES_ON_COMPONENT_DESTROY"
478481
},
@@ -533,6 +536,9 @@
533536
{
534537
"name": "Subscription"
535538
},
539+
{
540+
"name": "TEMPORARY_CONSUMER_NODE"
541+
},
536542
{
537543
"name": "TESTABILITY"
538544
},
@@ -803,6 +809,12 @@
803809
{
804810
"name": "configureViewWithDirective"
805811
},
812+
{
813+
"name": "consumerBeforeComputation"
814+
},
815+
{
816+
"name": "consumerDestroy"
817+
},
806818
{
807819
"name": "consumerIsLive"
808820
},
@@ -1529,6 +1541,9 @@
15291541
{
15301542
"name": "viewAttachedToChangeDetector"
15311543
},
1544+
{
1545+
"name": "viewShouldHaveReactiveConsumer"
1546+
},
15321547
{
15331548
"name": "visitDslNode"
15341549
},

0 commit comments

Comments
 (0)