Skip to content

Commit

Permalink
fix(core): properly update collection with repeated keys in @for (#52697
Browse files Browse the repository at this point in the history
)

This change fixes a bug in the new list reconcilation algorithm
that could lead to an infinite loop in certain situations.

More specifically, it adjusts the internal MultiMap implementation
such that an entry returned from the .get call is the same entry
(for an identical key) removed by the .delete call.

The existing logic of the MultiMap was leading to a situation where
one view was requested and attached to LContainer, but a very different
view was removed from the MultiMap. This was leaving an attached LView
in a collection that was supposed to hold only detached views.

Closes #52524

PR Close #52697
  • Loading branch information
pkozlowski-opensource authored and jessicajaniuk committed Nov 9, 2023
1 parent 2cea80c commit 44c48a4
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 3 deletions.
8 changes: 5 additions & 3 deletions packages/core/src/render3/list_reconciliation.ts
Expand Up @@ -238,8 +238,11 @@ export function reconcile<T, V>(
liveCollection.destroy(liveCollection.detach(liveEndIdx--));
}


// - destroy items that were detached but never attached again.
detachedItems?.forEach(item => liveCollection.destroy(item));
detachedItems?.forEach(item => {
liveCollection.destroy(item);
});
}

function attachPreviouslyDetached<T, V>(
Expand Down Expand Up @@ -285,8 +288,7 @@ class MultiMap<K, V> {
delete(key: K): boolean {
const listOfKeys = this.map.get(key);
if (listOfKeys !== undefined) {
// THINK: pop from the end or shift from the front? "Correct" vs. "slow".
listOfKeys.pop();
listOfKeys.shift();
return true;
}
return false;
Expand Down
46 changes: 46 additions & 0 deletions packages/core/test/acceptance/control_flow_for_spec.ts
Expand Up @@ -277,6 +277,52 @@ describe('control flow - for', () => {
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('5(0)|3(1)|7(2)|');
});

it('should correctly attach and detach views with duplicated keys', () => {
const BEFORE = [
{'name': 'Task 14', 'id': 14},
{'name': 'Task 14', 'id': 14},
{'name': 'Task 70', 'id': 70},
{'name': 'Task 34', 'id': 34},

];

const AFTER = [
{'name': 'Task 70', 'id': 70},
{'name': 'Task 14', 'id': 14},
{'name': 'Task 28', 'id': 28},
];

@Component({
standalone: true,
template: ``,
selector: 'child-cmp',
})
class ChildCmp {
}

@Component({
standalone: true,
imports: [ChildCmp],
template: `
@for(task of tasks; track task.id) {
<child-cmp/>
}
`,
})
class TestComponent {
tasks = BEFORE;
}

const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();

const cmp = fixture.componentInstance;
const nativeElement = fixture.debugElement.nativeElement;
cmp.tasks = AFTER;
fixture.detectChanges();
expect(nativeElement.querySelectorAll('child-cmp').length).toBe(3);
});
});

describe('content projection', () => {
Expand Down

0 comments on commit 44c48a4

Please sign in to comment.