From a8269264bf5698230eb991ca77c29374880a184b Mon Sep 17 00:00:00 2001 From: Sirui Chen <632882184@qq.com> Date: Thu, 17 May 2018 00:36:17 +0800 Subject: [PATCH] fix(core): make DefaultIterableDiffer keep the order of duplicates (#23941) 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 --- .../differs/default_iterable_differ.ts | 21 +++---- .../differs/default_iterable_differ_spec.ts | 59 +++++++++++++++++++ 2 files changed, 70 insertions(+), 10 deletions(-) diff --git a/packages/core/src/change_detection/differs/default_iterable_differ.ts b/packages/core/src/change_detection/differs/default_iterable_differ.ts index 91717994cc..60a1dacf8d 100644 --- 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 implements IterableDiffer, 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 index bc6a9040ea..ac244d0714 100644 --- 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);