fix(core): properly update collection with repeated keys in @for (#52697)

pkozlowski-opensource · thePunderWoman · commit 44c48a48358c · 2023-11-09T15:44:53.000Z
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
diff --git a/packages/core/src/render3/list_reconciliation.ts b/packages/core/src/render3/list_reconciliation.ts
@@ -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>(
@@ -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;
diff --git a/packages/core/test/acceptance/control_flow_for_spec.ts b/packages/core/test/acceptance/control_flow_for_spec.ts
@@ -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', () => {
