From aa389a691bc2e5726a0ded73d30962c29faab680 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Sat, 27 Sep 2025 07:57:45 -0700 Subject: [PATCH] fix(core): ensure `@for` iteration over field is reactive (#64113) When working with a proxy object such as signal forms' `Field`, accessing the `lenght` or `Symbol.iterator` may trgger a reactive read. This change ensures that `@for` properly captrues this before clearing the active consumer. PR Close #64113 --- .../src/render3/instructions/control_flow.ts | 8 +-- .../core/src/render3/list_reconciliation.ts | 9 +++ .../test/acceptance/control_flow_for_spec.ts | 59 +++++++++++++++++++ .../test/render3/list_reconciliation_spec.ts | 55 +++++++++-------- 4 files changed, 102 insertions(+), 29 deletions(-) diff --git a/packages/core/src/render3/instructions/control_flow.ts b/packages/core/src/render3/instructions/control_flow.ts index 09ab65cc379..0697f7cf8f7 100644 --- a/packages/core/src/render3/instructions/control_flow.ts +++ b/packages/core/src/render3/instructions/control_flow.ts @@ -39,15 +39,15 @@ import {NO_CHANGE} from '../tokens'; import {getConstant, getTNode} from '../util/view_utils'; import {createAndRenderEmbeddedLView, shouldAddViewToDom} from '../view_manipulation'; -import {declareNoDirectiveHostTemplate} from './template'; +import {AnimationLViewData} from '../../animation/interfaces'; +import {removeDehydratedViews} from '../../hydration/cleanup'; import { addLViewToLContainer, detachView, getLViewFromLContainer, removeLViewFromLContainer, } from '../view/container'; -import {removeDehydratedViews} from '../../hydration/cleanup'; -import {AnimationLViewData} from '../../animation/interfaces'; +import {declareNoDirectiveHostTemplate} from './template'; /** * Creates an LContainer for an ng-template representing a root node @@ -495,7 +495,7 @@ export function ɵɵrepeater(collection: Iterable | undefined | null): } const liveCollection = metadata.liveCollection; - reconcile(liveCollection, collection, metadata.trackByFn); + reconcile(liveCollection, collection, metadata.trackByFn, prevConsumer); // Warn developers about situations where the entire collection was re-created as part of the // reconciliation pass. Note that this warning might be "overreacting" and report cases where diff --git a/packages/core/src/render3/list_reconciliation.ts b/packages/core/src/render3/list_reconciliation.ts index 6cf93e89ddb..7e4f7c632ed 100644 --- a/packages/core/src/render3/list_reconciliation.ts +++ b/packages/core/src/render3/list_reconciliation.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.dev/license */ +import {setActiveConsumer, type ReactiveNode} from '@angular/core/primitives/signals'; import {TrackByFunction} from '../change_detection'; import {formatRuntimeError, RuntimeErrorCode} from '../errors'; @@ -103,11 +104,15 @@ function recordDuplicateKeys(keyToIdx: Map>, key: unknown, * @param newCollection the new, incoming collection; * @param trackByFn key generation function that determines equality between items in the life and * incoming collection; + * @param reactiveConsumer the reactive consumer to be used when accessing the length or iterator of + * the {@link newCollection}. This ensures that if the object is a proxy, reactive reads that + * occur when accessing the length or iterator are tracked. */ export function reconcile( liveCollection: LiveCollection, newCollection: Iterable | undefined | null, trackByFn: TrackByFunction, + reactiveConsumer: ReactiveNode | null, ): void { let detachedItems: UniqueValueMultiKeyMap | undefined = undefined; let liveKeysInTheFuture: Set | undefined = undefined; @@ -118,7 +123,9 @@ export function reconcile( const duplicateKeys = ngDevMode ? new Map>() : undefined; if (Array.isArray(newCollection)) { + setActiveConsumer(reactiveConsumer); let newEndIdx = newCollection.length - 1; + setActiveConsumer(null); while (liveStartIdx <= liveEndIdx && liveStartIdx <= newEndIdx) { // compare from the beginning @@ -235,7 +242,9 @@ export function reconcile( } } else if (newCollection != null) { // iterable - immediately fallback to the slow path + setActiveConsumer(reactiveConsumer); const newCollectionIterator = newCollection[Symbol.iterator](); + setActiveConsumer(null); let newIterationResult = newCollectionIterator.next(); while (!newIterationResult.done && liveStartIdx <= liveEndIdx) { const liveValue = liveCollection.at(liveStartIdx); diff --git a/packages/core/test/acceptance/control_flow_for_spec.ts b/packages/core/test/acceptance/control_flow_for_spec.ts index 08cea7eb695..92ad12d57b3 100644 --- a/packages/core/test/acceptance/control_flow_for_spec.ts +++ b/packages/core/test/acceptance/control_flow_for_spec.ts @@ -8,6 +8,7 @@ import {NgIf} from '@angular/common'; import { + ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, @@ -17,6 +18,7 @@ import { Pipe, PipeTransform, provideZoneChangeDetection, + signal, TemplateRef, ViewContainerRef, } from '../../src/core'; @@ -1103,4 +1105,61 @@ describe('control flow - for', () => { expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: 3'); }); }); + + describe('reactivity', () => { + it('should do a reactive read of length', () => { + const len = signal(2); + + @Component({ + template: `@for (item of items; track item) {{{item}}|}`, + changeDetection: ChangeDetectionStrategy.OnPush, + }) + class TestComponent { + items = new Proxy([1, 2, 3, 4, 5], { + get(target, prop) { + if (prop === 'length') { + return len(); + } + return target[prop as keyof number[]]; + }, + }); + } + + const fixture = TestBed.createComponent(TestComponent); + TestBed.tick(); + expect(fixture.nativeElement.textContent).toBe('1|2|'); + len.set(4); + TestBed.tick(); + expect(fixture.nativeElement.textContent).toBe('1|2|3|4|'); + }); + }); + + it('should do a reactive read of iterator', () => { + const iter = signal(() => [1, 2][Symbol.iterator]()); + + @Component({ + template: `@for (item of items; track $index) {{{item}}|}`, + changeDetection: ChangeDetectionStrategy.OnPush, + }) + class TestComponent { + items = new Proxy( + {}, + { + get(target, prop) { + if (prop === Symbol.iterator) { + return iter(); + } + return target[prop as keyof {}]; + }, + }, + ); + } + + const fixture = TestBed.createComponent(TestComponent); + TestBed.tick(); + expect(fixture.nativeElement.textContent).toBe('1|2|'); + iter.set(() => [1, 2, 3, 4][Symbol.iterator]()); + TestBed.tick(); + expect(fixture.nativeElement.textContent).toBe('1|2|3|4|'); + }); }); diff --git a/packages/core/test/render3/list_reconciliation_spec.ts b/packages/core/test/render3/list_reconciliation_spec.ts index 1aeaa857800..751a0787548 100644 --- a/packages/core/test/render3/list_reconciliation_spec.ts +++ b/packages/core/test/render3/list_reconciliation_spec.ts @@ -100,7 +100,7 @@ describe('list reconciliation', () => { describe('fast path', () => { it('should do nothing if 2 lists are the same', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, ['a', 'b', 'c'], trackByIdentity); + reconcile(pc, ['a', 'b', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([]); @@ -108,7 +108,7 @@ describe('list reconciliation', () => { it('should add items at the end', () => { const pc = new LoggingLiveCollection(['a', 'b']); - reconcile(pc, ['a', 'b', 'c'], trackByIdentity); + reconcile(pc, ['a', 'b', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([ @@ -119,7 +119,7 @@ describe('list reconciliation', () => { it('should swap items', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, ['c', 'b', 'a'], trackByIdentity); + reconcile(pc, ['c', 'b', 'a'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['c', 'b', 'a']); // TODO: think of expressing as swap @@ -133,7 +133,7 @@ describe('list reconciliation', () => { it('should should optimally swap adjacent items', () => { const pc = new LoggingLiveCollection(['a', 'b']); - reconcile(pc, ['b', 'a'], trackByIdentity); + reconcile(pc, ['b', 'a'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['b', 'a']); expect(pc.getLogs()).toEqual([ @@ -144,7 +144,7 @@ describe('list reconciliation', () => { it('should detect moves to the front', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c', 'd']); - reconcile(pc, ['a', 'd', 'b', 'c'], trackByIdentity); + reconcile(pc, ['a', 'd', 'b', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'd', 'b', 'c']); expect(pc.getLogs()).toEqual([ @@ -155,7 +155,7 @@ describe('list reconciliation', () => { it('should delete items in the middle', () => { const pc = new LoggingLiveCollection(['a', 'x', 'b', 'c']); - reconcile(pc, ['a', 'b', 'c'], trackByIdentity); + reconcile(pc, ['a', 'b', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([ @@ -166,7 +166,7 @@ describe('list reconciliation', () => { it('should delete items from the beginning', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, ['c'], trackByIdentity); + reconcile(pc, ['c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['c']); expect(pc.getLogs()).toEqual([ @@ -179,7 +179,7 @@ describe('list reconciliation', () => { it('should delete items from the end', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, ['a'], trackByIdentity); + reconcile(pc, ['a'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a']); expect(pc.getLogs()).toEqual([ @@ -192,7 +192,7 @@ describe('list reconciliation', () => { it('should work with duplicated items', () => { const pc = new LoggingLiveCollection(['a', 'a', 'a']); - reconcile(pc, ['a', 'a', 'a'], trackByIdentity); + reconcile(pc, ['a', 'a', 'a'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'a', 'a']); expect(pc.getLogs()).toEqual([]); @@ -202,7 +202,7 @@ describe('list reconciliation', () => { describe('slow path', () => { it('should delete multiple items from the middle', () => { const pc = new LoggingLiveCollection(['a', 'x1', 'b', 'x2', 'c']); - reconcile(pc, ['a', 'b', 'c'], trackByIdentity); + reconcile(pc, ['a', 'b', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([ @@ -215,7 +215,7 @@ describe('list reconciliation', () => { it('should add multiple items in the middle', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, ['a', 'n1', 'b', 'n2', 'c'], trackByIdentity); + reconcile(pc, ['a', 'n1', 'b', 'n2', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'n1', 'b', 'n2', 'c']); expect(pc.getLogs()).toEqual([ @@ -228,7 +228,7 @@ describe('list reconciliation', () => { it('should go back to the fast path when start / end is different', () => { const pc = new LoggingLiveCollection(['s1', 'a', 'b', 'c', 'e1']); - reconcile(pc, ['s2', 'a', 'b', 'c', 'e2'], trackByIdentity); + reconcile(pc, ['s2', 'a', 'b', 'c', 'e2'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['s2', 'a', 'b', 'c', 'e2']); expect(pc.getLogs()).toEqual([ @@ -250,7 +250,7 @@ describe('list reconciliation', () => { it('should detect moves to the back', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c', 'd']); - reconcile(pc, ['b', 'c', 'n1', 'n2', 'n3', 'a', 'd'], trackByIdentity); + reconcile(pc, ['b', 'c', 'n1', 'n2', 'n3', 'a', 'd'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['b', 'c', 'n1', 'n2', 'n3', 'a', 'd']); expect(pc.getLogs()).toEqual([ ['detach', 0, 'a'], @@ -272,7 +272,7 @@ describe('list reconciliation', () => { {k: 2}, {k: 3}, ]); - reconcile(pc, [{k: 2}, {k: 3}, {k: 1}, {k: 1}, {k: 1}, {k: 4}], trackByKey); + reconcile(pc, [{k: 2}, {k: 3}, {k: 1}, {k: 1}, {k: 1}, {k: 4}], trackByKey, null); expect(pc.getCollection()).toEqual([{k: 2}, {k: 3}, {k: 1}, {k: 1}, {k: 1}, {k: 4}]); expect(pc.getLogs()).toEqual([ @@ -291,7 +291,7 @@ describe('list reconciliation', () => { describe('iterables', () => { it('should do nothing if 2 lists represented as iterables are the same', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, new Set(['a', 'b', 'c']), trackByIdentity); + reconcile(pc, new Set(['a', 'b', 'c']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([]); @@ -299,7 +299,7 @@ describe('list reconciliation', () => { it('should add items at the end', () => { const pc = new LoggingLiveCollection(['a', 'b']); - reconcile(pc, new Set(['a', 'b', 'c']), trackByIdentity); + reconcile(pc, new Set(['a', 'b', 'c']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([ @@ -310,7 +310,7 @@ describe('list reconciliation', () => { it('should add multiple items in the middle', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, new Set(['a', 'n1', 'b', 'n2', 'c']), trackByIdentity); + reconcile(pc, new Set(['a', 'n1', 'b', 'n2', 'c']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'n1', 'b', 'n2', 'c']); expect(pc.getLogs()).toEqual([ @@ -323,7 +323,7 @@ describe('list reconciliation', () => { it('should delete items from the end', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, new Set(['a']), trackByIdentity); + reconcile(pc, new Set(['a']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['a']); expect(pc.getLogs()).toEqual([ @@ -336,7 +336,7 @@ describe('list reconciliation', () => { it('should detect (slow) moves to the front', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c', 'd']); - reconcile(pc, new Set(['a', 'd', 'b', 'c']), trackByIdentity); + reconcile(pc, new Set(['a', 'd', 'b', 'c']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'd', 'b', 'c']); expect(pc.getLogs()).toEqual([ @@ -349,7 +349,7 @@ describe('list reconciliation', () => { it('should detect (fast) moves to the back', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c', 'd']); - reconcile(pc, new Set(['b', 'c', 'a', 'd']), trackByIdentity); + reconcile(pc, new Set(['b', 'c', 'a', 'd']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['b', 'c', 'a', 'd']); expect(pc.getLogs()).toEqual([ ['detach', 0, 'a'], @@ -360,11 +360,11 @@ describe('list reconciliation', () => { it('should allow switching collection types', () => { const pc = new LoggingLiveCollection(['a', 'b', 'c']); - reconcile(pc, new Set(['a', 'b', 'c']), trackByIdentity); + reconcile(pc, new Set(['a', 'b', 'c']), trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([]); - reconcile(pc, ['a', 'b', 'c'], trackByIdentity); + reconcile(pc, ['a', 'b', 'c'], trackByIdentity, null); expect(pc.getCollection()).toEqual(['a', 'b', 'c']); expect(pc.getLogs()).toEqual([]); }); @@ -405,14 +405,14 @@ describe('list reconciliation', () => { it('should update when tracking by index - fast path from the start', () => { const pc = new LoggingLiveCollection([], new RepeaterLikeItemFactory()); - reconcile(pc, ['a', 'b', 'c'], trackByIndex); + reconcile(pc, ['a', 'b', 'c'], trackByIndex, null); expect(pc.getCollection()).toEqual([ {index: 0, implicit: 'a'}, {index: 1, implicit: 'b'}, {index: 2, implicit: 'c'}, ]); - reconcile(pc, ['c', 'b', 'a'], trackByIndex); + reconcile(pc, ['c', 'b', 'a'], trackByIndex, null); expect(pc.getCollection()).toEqual([ {index: 0, implicit: 'c'}, {index: 1, implicit: 'b'}, @@ -426,7 +426,7 @@ describe('list reconciliation', () => { new RepeaterLikeItemFactory>(), ); - reconcile(pc, [{k: 'o', v: 'o'}], trackByKey); + reconcile(pc, [{k: 'o', v: 'o'}], trackByKey, null); expect(pc.getCollection()).toEqual([{index: 0, implicit: {k: 'o', v: 'o'}}]); reconcile( @@ -436,6 +436,7 @@ describe('list reconciliation', () => { {k: 'o', v: 'oo'}, ], trackByKey, + null, ); expect(pc.getCollection()).toEqual([ {index: 0, implicit: {k: 'n', v: 'n'}}, @@ -458,6 +459,7 @@ describe('list reconciliation', () => { {k: 2, v: 'c'}, ], trackByKey as any, + null, ); expect(pc.getCollection()).toEqual([ {index: 0, implicit: {k: 0, v: 'a'}}, @@ -473,6 +475,7 @@ describe('list reconciliation', () => { {k: 0, v: 'aa'}, ], trackByKey as any, + null, ); expect(pc.getCollection()).toEqual([ {index: 0, implicit: {k: 2, v: 'cc'}}, @@ -495,6 +498,7 @@ describe('list reconciliation', () => { {k: 3, v: 'd'}, ], trackByKey as any, + null, ); expect(pc.getCollection()).toEqual([ {index: 0, implicit: {k: 0, v: 'a'}}, @@ -512,6 +516,7 @@ describe('list reconciliation', () => { {k: 2, v: 'cc'}, ], trackByKey as any, + null, ); expect(pc.getCollection()).toEqual([ {index: 0, implicit: {k: 0, v: 'aa'}},