fix(core): make DefaultIterableDiffer keep the order of duplicates (#23941)

csr632 · thePunderWoman · commit a8269264bf56 · 2021-01-26T15:44:42.000-08:00
Previously, in `_mismatch()`, the `DefaultIterableDiffer` first checks `_linkedRecords` for `itemTrackBy`, then checks `_unlinkedRecords`. This cause the `DefaultIterableDiffer` to move "later" items that match the `itemTrackBy` from the old collection, rather than using the "earlier" one. Now we check `_unlinkedRecords` first, so that the `DefaultIterableDiffer` can give a more stable and reasonable result after diffing. For example, rather than (`a1` and `a2` have same trackById) ``` a1 b c a2 => b a2 c a1 ``` we get ``` a1 b c a2 => b a1 c a2 ``` where a1 and a2 retain their original order despite both having the same track by value. Fixes #23815 PR Close #23941
diff --git a/packages/core/src/change_detection/differs/default_iterable_differ.ts b/packages/core/src/change_detection/differs/default_iterable_differ.ts
@@ -281,23 +281,24 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
       this._remove(record);
     }
 
-    // Attempt to see if we have seen the item before.
-    record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
+    // See if we have evicted the item, which used to be at some anterior position of _itHead list.
+    record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy, null);
     if (record !== null) {
-      // We have seen this before, we need to move it forward in the collection.
-      // But first we need to check if identity changed, so we can update in view if necessary
+      // It is an item which we have evicted earlier: reinsert it back into the list.
+      // But first we need to check if identity changed, so we can update in view if necessary.
       if (!Object.is(record.item, item)) this._addIdentityChange(record, item);
 
-      this._moveAfter(record, previousRecord, index);
+      this._reinsertAfter(record, previousRecord, index);
     } else {
-      // Never seen it, check evicted list.
-      record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy, null);
+      // Attempt to see if the item is at some posterior position of _itHead list.
+      record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
       if (record !== null) {
-        // It is an item which we have evicted earlier: reinsert it back into the list.
-        // But first we need to check if identity changed, so we can update in view if necessary
+        // We have the item in _itHead at/after `index` position. We need to move it forward in the
+        // collection.
+        // But first we need to check if identity changed, so we can update in view if necessary.
         if (!Object.is(record.item, item)) this._addIdentityChange(record, item);
 
-        this._reinsertAfter(record, previousRecord, index);
+        this._moveAfter(record, previousRecord, index);
       } else {
         // It is a new item: add it.
         record =
diff --git a/packages/core/test/change_detection/differs/default_iterable_differ_spec.ts b/packages/core/test/change_detection/differs/default_iterable_differ_spec.ts
@@ -561,6 +561,65 @@ class ComplexItem {
         }));
       });
 
+      it('should keep the order of duplicates', () => {
+        const l1 = [
+          new ComplexItem('a', 'blue'),
+          new ComplexItem('b', 'yellow'),
+          new ComplexItem('c', 'orange'),
+          new ComplexItem('a', 'red'),
+        ];
+        differ.check(l1);
+
+        const l2 = [
+          new ComplexItem('b', 'yellow'),
+          new ComplexItem('a', 'blue'),
+          new ComplexItem('c', 'orange'),
+          new ComplexItem('a', 'red'),
+        ];
+        differ.check(l2);
+
+        expect(iterableDifferToString(differ)).toEqual(iterableChangesAsString({
+          collection: [
+            '{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]', '{id: c, color: orange}',
+            '{id: a, color: red}'
+          ],
+          identityChanges: [
+            '{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]', '{id: c, color: orange}',
+            '{id: a, color: red}'
+          ],
+          previous: [
+            '{id: a, color: blue}[0->1]', '{id: b, color: yellow}[1->0]', '{id: c, color: orange}',
+            '{id: a, color: red}'
+          ],
+          moves: ['{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]'],
+        }));
+      });
+
+      it('should not have identity changed', () => {
+        const l1 = [
+          new ComplexItem('a', 'blue'),
+          new ComplexItem('b', 'yellow'),
+          new ComplexItem('c', 'orange'),
+          new ComplexItem('a', 'red'),
+        ];
+        differ.check(l1);
+
+        const l2 = [l1[1], l1[0], l1[2], l1[3]];
+        differ.check(l2);
+
+        expect(iterableDifferToString(differ)).toEqual(iterableChangesAsString({
+          collection: [
+            '{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]', '{id: c, color: orange}',
+            '{id: a, color: red}'
+          ],
+          previous: [
+            '{id: a, color: blue}[0->1]', '{id: b, color: yellow}[1->0]', '{id: c, color: orange}',
+            '{id: a, color: red}'
+          ],
+          moves: ['{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]'],
+        }));
+      });
+
       it('should track removals normally', () => {
         const l = buildItemList(['a', 'b', 'c']);
         differ.check(l);
