fix(core): fix the key/value differ (#15539)

fixes #15457

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

@@ -27,13 +27,20 @@ export class DefaultKeyValueDifferFactory<K, V> implements KeyValueDifferFactory
 }
 
 export class DefaultKeyValueDiffer<K, V> implements KeyValueDiffer<K, V>, KeyValueChanges<K, V> {
-  private _records: Map<K, V> = new Map<K, V>();
+  private _records = new Map<K, KeyValueChangeRecord_<K, V>>();
+
   private _mapHead: KeyValueChangeRecord_<K, V> = null;
+  // _appendAfter is used in the check loop
+  private _appendAfter: KeyValueChangeRecord_<K, V> = null;
+
   private _previousMapHead: KeyValueChangeRecord_<K, V> = null;
+
   private _changesHead: KeyValueChangeRecord_<K, V> = null;
   private _changesTail: KeyValueChangeRecord_<K, V> = null;
+
   private _additionsHead: KeyValueChangeRecord_<K, V> = null;
   private _additionsTail: KeyValueChangeRecord_<K, V> = null;
+
   private _removalsHead: KeyValueChangeRecord_<K, V> = null;
   private _removalsTail: KeyValueChangeRecord_<K, V> = null;
 
@@ -90,67 +97,130 @@ export class DefaultKeyValueDiffer<K, V> implements KeyValueDiffer<K, V>, KeyVal
 
   onDestroy() {}
 
+  /**
+   * Check the current state of the map vs the previous.
+   * The algorithm is optimised for when the keys do no change.
+   */
   check(map: Map<any, any>|{[k: string]: any}): boolean {
     this._reset();
-    const records = this._records;
+
-    let oldSeqRecord: KeyValueChangeRecord_<K, V> = this._mapHead;
+    let insertBefore = this._mapHead;
-    let lastOldSeqRecord: KeyValueChangeRecord_<K, V> = null;
+    this._appendAfter = null;
-    let lastNewSeqRecord: KeyValueChangeRecord_<K, V> = null;
-    let seqChanged: boolean = false;
 
     this._forEach(map, (value: any, key: any) => {
-      let newSeqRecord: any;
+      if (insertBefore && insertBefore.key === key) {
-      if (oldSeqRecord && key === oldSeqRecord.key) {
+        this._maybeAddToChanges(insertBefore, value);
-        newSeqRecord = oldSeqRecord;
+        this._appendAfter = insertBefore;
-        this._maybeAddToChanges(newSeqRecord, value);
+        insertBefore = insertBefore._next;
       } else {
-        seqChanged = true;
+        const record = this._getOrCreateRecordForKey(key, value);
-        if (oldSeqRecord !== null) {
+        insertBefore = this._insertBeforeOrAppend(insertBefore, record);
-          this._removeFromSeq(lastOldSeqRecord, oldSeqRecord);
+      }
-          this._addToRemovals(oldSeqRecord);
+    });
-        }
+
-        if (records.has(key)) {
+    // Items remaining at the end of the list have been deleted
-          newSeqRecord = records.get(key);
+    if (insertBefore) {
-          this._maybeAddToChanges(newSeqRecord, value);
+      if (insertBefore._prev) {
-        } else {
+        insertBefore._prev._next = null;
-          newSeqRecord = new KeyValueChangeRecord_<K, V>(key);
-          records.set(key, newSeqRecord);
-          newSeqRecord.currentValue = value;
-          this._addToAdditions(newSeqRecord);
-        }
       }
 
-      if (seqChanged) {
+      this._removalsHead = insertBefore;
-        if (this._isInRemovals(newSeqRecord)) {
+      this._removalsTail = insertBefore;
-          this._removeFromRemovals(newSeqRecord);
+
-        }
+      for (let record = insertBefore; record !== null; record = record._nextRemoved) {
-        if (lastNewSeqRecord == null) {
+        if (record === this._mapHead) {
-          this._mapHead = newSeqRecord;
+          this._mapHead = null;
-        } else {
-          lastNewSeqRecord._next = newSeqRecord;
         }
+        this._records.delete(record.key);
+        record._nextRemoved = record._next;
+        record.previousValue = record.currentValue;
+        record.currentValue = null;
+        record._prev = null;
+        record._next = null;
       }
-      lastOldSeqRecord = oldSeqRecord;
+    }
-      lastNewSeqRecord = newSeqRecord;
+
-      oldSeqRecord = oldSeqRecord && oldSeqRecord._next;
-    });
-    this._truncate(lastOldSeqRecord, oldSeqRecord);
     return this.isDirty;
   }
 
+  /**
+   * Inserts a record before `before` or append at the end of the list when `before` is null.
+   *
+   * Notes:
+   * - This method appends at `this._appendAfter`,
+   * - This method updates `this._appendAfter`,
+   * - The return value is the new value for the insertion pointer.
+   */
+  private _insertBeforeOrAppend(
+      before: KeyValueChangeRecord_<K, V>,
+      record: KeyValueChangeRecord_<K, V>): KeyValueChangeRecord_<K, V> {
+    if (before) {
+      const prev = before._prev;
+      record._next = before;
+      record._prev = prev;
+      before._prev = record;
+      if (prev) {
+        prev._next = record;
+      }
+      if (before === this._mapHead) {
+        this._mapHead = record;
+      }
+
+      this._appendAfter = before;
+      return before;
+    }
+
+    if (this._appendAfter) {
+      this._appendAfter._next = record;
+      record._prev = this._appendAfter;
+    } else {
+      this._mapHead = record;
+    }
+
+    this._appendAfter = record;
+    return null;
+  }
+
+  private _getOrCreateRecordForKey(key: K, value: V): KeyValueChangeRecord_<K, V> {
+    if (this._records.has(key)) {
+      const record = this._records.get(key);
+      this._maybeAddToChanges(record, value);
+      const prev = record._prev;
+      const next = record._next;
+      if (prev) {
+        prev._next = next;
+      }
+      if (next) {
+        next._prev = prev;
+      }
+      record._next = null;
+      record._prev = null;
+
+      return record;
+    }
+
+    const record = new KeyValueChangeRecord_<K, V>(key);
+    this._records.set(key, record);
+    record.currentValue = value;
+    this._addToAdditions(record);
+    return record;
+  }
+
   /** @internal */
   _reset() {
     if (this.isDirty) {
       let record: KeyValueChangeRecord_<K, V>;
-      // Record the state of the mapping
+      // let `_previousMapHead` contain the state of the map before the changes
-      for (record = this._previousMapHead = this._mapHead; record !== null; record = record._next) {
+      this._previousMapHead = this._mapHead;
+      for (record = this._previousMapHead; record !== null; record = record._next) {
         record._nextPrevious = record._next;
       }
 
+      // Update `record.previousValue` with the value of the item before the changes
+      // We need to update all changed items (that's those which have been added and changed)
       for (record = this._changesHead; record !== null; record = record._nextChanged) {
         record.previousValue = record.currentValue;
       }
-
       for (record = this._additionsHead; record != null; record = record._nextAdded) {
         record.previousValue = record.currentValue;
       }
@@ -161,27 +231,7 @@ export class DefaultKeyValueDiffer<K, V> implements KeyValueDiffer<K, V>, KeyVal
     }
   }
 
-  private _truncate(lastRecord: KeyValueChangeRecord_<K, V>, record: KeyValueChangeRecord_<K, V>) {
+  // Add the record or a given key to the list of changes only when the value has actually changed
-    while (record !== null) {
-      if (lastRecord === null) {
-        this._mapHead = null;
-      } else {
-        lastRecord._next = null;
-      }
-      const nextRecord = record._next;
-      this._addToRemovals(record);
-      lastRecord = record;
-      record = nextRecord;
-    }
-
-    for (let rec: KeyValueChangeRecord_<K, V> = this._removalsHead; rec !== null;
-         rec = rec._nextRemoved) {
-      rec.previousValue = rec.currentValue;
-      rec.currentValue = null;
-      this._records.delete(rec.key);
-    }
-  }
-
   private _maybeAddToChanges(record: KeyValueChangeRecord_<K, V>, newValue: any): void {
     if (!looseIdentical(newValue, record.currentValue)) {
       record.previousValue = record.currentValue;
@@ -190,47 +240,6 @@ export class DefaultKeyValueDiffer<K, V> implements KeyValueDiffer<K, V>, KeyVal
     }
   }
 
-  private _isInRemovals(record: KeyValueChangeRecord_<K, V>) {
-    return record === this._removalsHead || record._nextRemoved !== null ||
-        record._prevRemoved !== null;
-  }
-
-  private _addToRemovals(record: KeyValueChangeRecord_<K, V>) {
-    if (this._removalsHead === null) {
-      this._removalsHead = this._removalsTail = record;
-    } else {
-      this._removalsTail._nextRemoved = record;
-      record._prevRemoved = this._removalsTail;
-      this._removalsTail = record;
-    }
-  }
-
-  private _removeFromSeq(prev: KeyValueChangeRecord_<K, V>, record: KeyValueChangeRecord_<K, V>) {
-    const next = record._next;
-    if (prev === null) {
-      this._mapHead = next;
-    } else {
-      prev._next = next;
-    }
-    record._next = null;
-  }
-
-  private _removeFromRemovals(record: KeyValueChangeRecord_<K, V>) {
-    const prev = record._prevRemoved;
-    const next = record._nextRemoved;
-    if (prev === null) {
-      this._removalsHead = next;
-    } else {
-      prev._nextRemoved = next;
-    }
-    if (next === null) {
-      this._removalsTail = prev;
-    } else {
-      next._prevRemoved = prev;
-    }
-    record._prevRemoved = record._nextRemoved = null;
-  }
-
   private _addToAdditions(record: KeyValueChangeRecord_<K, V>) {
     if (this._additionsHead === null) {
       this._additionsHead = this._additionsTail = record;
@@ -303,12 +312,12 @@ class KeyValueChangeRecord_<K, V> implements KeyValueChangeRecord<K, V> {
   /** @internal */
   _next: KeyValueChangeRecord_<K, V> = null;
   /** @internal */
+  _prev: KeyValueChangeRecord_<K, V> = null;
+  /** @internal */
   _nextAdded: KeyValueChangeRecord_<K, V> = null;
   /** @internal */
   _nextRemoved: KeyValueChangeRecord_<K, V> = null;
   /** @internal */
-  _prevRemoved: KeyValueChangeRecord_<K, V> = null;
-  /** @internal */
   _nextChanged: KeyValueChangeRecord_<K, V> = null;
 
   constructor(public key: K) {}

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

@@ -7,7 +7,6 @@
  */
 
 import {DefaultKeyValueDiffer, DefaultKeyValueDifferFactory} from '@angular/core/src/change_detection/differs/default_keyvalue_differ';
-import {afterEach, beforeEach, describe, expect, it} from '@angular/core/testing/src/testing_internal';
 import {kvChangesAsString} from '../../change_detection/util';
 
 // todo(vicb): Update the code & tests for object equality
@@ -55,21 +54,17 @@ export function main() {
       });
 
       it('should expose previous and current value', () => {
-        let previous: any /** TODO #9100 */, current: any /** TODO #9100 */;
-
         m.set(1, 10);
         differ.check(m);
 
         m.set(1, 20);
         differ.check(m);
 
-        differ.forEachChangedItem((record: any /** TODO #9100 */) => {
+        differ.forEachChangedItem((record: any) => {
-          previous = record.previousValue;
+          expect(record.previousValue).toEqual(10);
-          current = record.currentValue;
+          expect(record.currentValue).toEqual(20);
         });
 
-        expect(previous).toEqual(10);
-        expect(current).toEqual(20);
       });
 
       it('should do basic map watching', () => {
@@ -198,6 +193,19 @@ export function main() {
             changes: ['b[0->1]', 'a[0->1]']
           }));
         });
+
+        it('should when the first item is moved', () => {
+          differ.check({a: 'a', b: 'b'});
+          differ.check({c: 'c', a: 'a'});
+
+          expect(differ.toString()).toEqual(kvChangesAsString({
+            map: ['c[null->c]', 'a'],
+            previous: ['a', 'b[b->null]'],
+            additions: ['c[null->c]'],
+            removals: ['b[b->null]']
+          }));
+        });
+
       });
 
       describe('diff', () => {