Skip to content

Commit a826926

Browse files
csr632thePunderWoman
authored andcommitted
fix(core): make DefaultIterableDiffer keep the order of duplicates (angular#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 angular#23815 PR Close angular#23941
1 parent cffb00e commit a826926

File tree

2 files changed

+70
-10
lines changed

2 files changed

+70
-10
lines changed

packages/core/src/change_detection/differs/default_iterable_differ.ts

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -281,23 +281,24 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
281281
this._remove(record);
282282
}
283283

284-
// Attempt to see if we have seen the item before.
285-
record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
284+
// See if we have evicted the item, which used to be at some anterior position of _itHead list.
285+
record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy, null);
286286
if (record !== null) {
287-
// We have seen this before, we need to move it forward in the collection.
288-
// But first we need to check if identity changed, so we can update in view if necessary
287+
// It is an item which we have evicted earlier: reinsert it back into the list.
288+
// But first we need to check if identity changed, so we can update in view if necessary.
289289
if (!Object.is(record.item, item)) this._addIdentityChange(record, item);
290290

291-
this._moveAfter(record, previousRecord, index);
291+
this._reinsertAfter(record, previousRecord, index);
292292
} else {
293-
// Never seen it, check evicted list.
294-
record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy, null);
293+
// Attempt to see if the item is at some posterior position of _itHead list.
294+
record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
295295
if (record !== null) {
296-
// It is an item which we have evicted earlier: reinsert it back into the list.
297-
// But first we need to check if identity changed, so we can update in view if necessary
296+
// We have the item in _itHead at/after `index` position. We need to move it forward in the
297+
// collection.
298+
// But first we need to check if identity changed, so we can update in view if necessary.
298299
if (!Object.is(record.item, item)) this._addIdentityChange(record, item);
299300

300-
this._reinsertAfter(record, previousRecord, index);
301+
this._moveAfter(record, previousRecord, index);
301302
} else {
302303
// It is a new item: add it.
303304
record =

packages/core/test/change_detection/differs/default_iterable_differ_spec.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,65 @@ class ComplexItem {
561561
}));
562562
});
563563

564+
it('should keep the order of duplicates', () => {
565+
const l1 = [
566+
new ComplexItem('a', 'blue'),
567+
new ComplexItem('b', 'yellow'),
568+
new ComplexItem('c', 'orange'),
569+
new ComplexItem('a', 'red'),
570+
];
571+
differ.check(l1);
572+
573+
const l2 = [
574+
new ComplexItem('b', 'yellow'),
575+
new ComplexItem('a', 'blue'),
576+
new ComplexItem('c', 'orange'),
577+
new ComplexItem('a', 'red'),
578+
];
579+
differ.check(l2);
580+
581+
expect(iterableDifferToString(differ)).toEqual(iterableChangesAsString({
582+
collection: [
583+
'{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]', '{id: c, color: orange}',
584+
'{id: a, color: red}'
585+
],
586+
identityChanges: [
587+
'{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]', '{id: c, color: orange}',
588+
'{id: a, color: red}'
589+
],
590+
previous: [
591+
'{id: a, color: blue}[0->1]', '{id: b, color: yellow}[1->0]', '{id: c, color: orange}',
592+
'{id: a, color: red}'
593+
],
594+
moves: ['{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]'],
595+
}));
596+
});
597+
598+
it('should not have identity changed', () => {
599+
const l1 = [
600+
new ComplexItem('a', 'blue'),
601+
new ComplexItem('b', 'yellow'),
602+
new ComplexItem('c', 'orange'),
603+
new ComplexItem('a', 'red'),
604+
];
605+
differ.check(l1);
606+
607+
const l2 = [l1[1], l1[0], l1[2], l1[3]];
608+
differ.check(l2);
609+
610+
expect(iterableDifferToString(differ)).toEqual(iterableChangesAsString({
611+
collection: [
612+
'{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]', '{id: c, color: orange}',
613+
'{id: a, color: red}'
614+
],
615+
previous: [
616+
'{id: a, color: blue}[0->1]', '{id: b, color: yellow}[1->0]', '{id: c, color: orange}',
617+
'{id: a, color: red}'
618+
],
619+
moves: ['{id: b, color: yellow}[1->0]', '{id: a, color: blue}[0->1]'],
620+
}));
621+
});
622+
564623
it('should track removals normally', () => {
565624
const l = buildItemList(['a', 'b', 'c']);
566625
differ.check(l);

0 commit comments

Comments
 (0)