fix(core): fix re-insertions in the iterable differ (#17891)

fixes #17852

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

@@ -172,7 +172,6 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
 
   onDestroy() {}
 
-  // todo(vicb): optim for UnmodifiableListView (frozen arrays)
   check(collection: NgIterable<V>): boolean {
     this._reset();
 
@@ -281,12 +280,12 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
   _mismatch(record: IterableChangeRecord_<V>|null, item: V, itemTrackBy: any, index: number):
       IterableChangeRecord_<V> {
     // The previous record after which we will append the current one.
-    let previousRecord: IterableChangeRecord_<V>;
+    let previousRecord: IterableChangeRecord_<V>|null;
 
     if (record === null) {
-      previousRecord = this._itTail !;
+      previousRecord = this._itTail;
     } else {
-      previousRecord = record._prev !;
+      previousRecord = record._prev;
       // Remove the record from the collection since we know it does not match the item.
       this._remove(record);
     }
@@ -394,7 +393,7 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
 
   /** @internal */
   _reinsertAfter(
-      record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>,
+      record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>|null,
       index: number): IterableChangeRecord_<V> {
     if (this._unlinkedRecords !== null) {
       this._unlinkedRecords.remove(record);
@@ -419,8 +418,9 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
   }
 
   /** @internal */
-  _moveAfter(record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>, index: number):
+  _moveAfter(
-      IterableChangeRecord_<V> {
+      record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>|null,
+      index: number): IterableChangeRecord_<V> {
     this._unlink(record);
     this._insertAfter(record, prevRecord, index);
     this._addToMoves(record, index);
@@ -428,8 +428,9 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
   }
 
   /** @internal */
-  _addAfter(record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>, index: number):
+  _addAfter(
-      IterableChangeRecord_<V> {
+      record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>|null,
+      index: number): IterableChangeRecord_<V> {
     this._insertAfter(record, prevRecord, index);
 
     if (this._additionsTail === null) {
@@ -447,7 +448,7 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
 
   /** @internal */
   _insertAfter(
-      record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>,
+      record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>|null,
       index: number): IterableChangeRecord_<V> {
     // todo(vicb)
     // assert(record != prevRecord);
@@ -665,11 +666,11 @@ class _DuplicateItemRecordList<V> {
   }
 
   // Returns a IterableChangeRecord_ having IterableChangeRecord_.trackById == trackById and
-  // IterableChangeRecord_.currentIndex >= afterIndex
+  // IterableChangeRecord_.currentIndex >= atOrAfterIndex
-  get(trackById: any, afterIndex: number|null): IterableChangeRecord_<V>|null {
+  get(trackById: any, atOrAfterIndex: number|null): IterableChangeRecord_<V>|null {
     let record: IterableChangeRecord_<V>|null;
     for (record = this._head; record !== null; record = record._nextDup) {
-      if ((afterIndex === null || afterIndex < record.currentIndex !) &&
+      if ((atOrAfterIndex === null || atOrAfterIndex <= record.currentIndex !) &&
           looseIdentical(record.trackById, trackById)) {
         return record;
       }
@@ -724,15 +725,15 @@ class _DuplicateMap<V> {
 
   /**
    * Retrieve the `value` using key. Because the IterableChangeRecord_ value may be one which we
-   * have already iterated over, we use the afterIndex to pretend it is not there.
+   * have already iterated over, we use the `atOrAfterIndex` to pretend it is not there.
    *
    * Use case: `[a, b, c, a, a]` if we are at index `3` which is the second `a` then asking if we
-   * have any more `a`s needs to return the last `a` not the first or second.
+   * have any more `a`s needs to return the second `a`.
    */
-  get(trackById: any, afterIndex: number|null): IterableChangeRecord_<V>|null {
+  get(trackById: any, atOrAfterIndex: number|null): IterableChangeRecord_<V>|null {
     const key = trackById;
     const recordList = this.map.get(key);
-    return recordList ? recordList.get(trackById, afterIndex) : null;
+    return recordList ? recordList.get(trackById, atOrAfterIndex) : null;
   }
 
   /**

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

@@ -10,7 +10,7 @@ import {Optional, Provider, SkipSelf} from '../../di';
 import {ChangeDetectorRef} from '../change_detector_ref';
 
 /**
- * A type describing supported interable types.
+ * A type describing supported iterable types.
  *
  * @stable
  */

packages/core/test/change_detection/change_detector_util_spec.ts

@@ -7,7 +7,6 @@
  */
 
 import {devModeEqual} from '@angular/core/src/change_detection/change_detection_util';
-import {describe, expect, it} from '@angular/core/testing/src/testing_internal';
 
 export function main() {
   describe('ChangeDetectionUtil', () => {

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

@@ -7,7 +7,6 @@
  */
 
 import {DefaultIterableDiffer, DefaultIterableDifferFactory} from '@angular/core/src/change_detection/differs/default_iterable_differ';
-import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testing_internal';
 
 import {TestIterable} from '../../change_detection/iterable';
 import {iterableChangesAsString} from '../../change_detection/util';
@@ -28,7 +27,7 @@ class ComplexItem {
 export function main() {
   describe('iterable differ', function() {
     describe('DefaultIterableDiffer', function() {
-      let differ: any /** TODO #9100 */;
+      let differ: DefaultIterableDiffer<any>;
 
       beforeEach(() => { differ = new DefaultIterableDiffer(); });
 
@@ -41,7 +40,7 @@ export function main() {
       });
 
       it('should support iterables', () => {
-        const l = new TestIterable();
+        const l: any = new TestIterable();
 
         differ.check(l);
         expect(differ.toString()).toEqual(iterableChangesAsString({collection: []}));
@@ -64,7 +63,7 @@ export function main() {
       });
 
       it('should detect additions', () => {
-        const l: any[] /** TODO #9100 */ = [];
+        const l: any[] = [];
         differ.check(l);
         expect(differ.toString()).toEqual(iterableChangesAsString({collection: []}));
 
@@ -144,7 +143,7 @@ export function main() {
       });
 
       it('should detect changes in list', () => {
-        const l: any[] /** TODO #9100 */ = [];
+        const l: any[] = [];
         differ.check(l);
 
         l.push('a');
@@ -192,19 +191,7 @@ export function main() {
         }));
       });
 
-      it('should test string by value rather than by reference (Dart)', () => {
+      it('should ignore [NaN] != [NaN]', () => {
-        const l = ['a', 'boo'];
-        differ.check(l);
-
-        const b = 'b';
-        const oo = 'oo';
-        l[1] = b + oo;
-        differ.check(l);
-        expect(differ.toString())
-            .toEqual(iterableChangesAsString({collection: ['a', 'boo'], previous: ['a', 'boo']}));
-      });
-
-      it('should ignore [NaN] != [NaN] (JS)', () => {
         const l = [NaN];
         differ.check(l);
         differ.check(l);
@@ -294,6 +281,22 @@ export function main() {
         }));
       });
 
+      // https://github.com/angular/angular/issues/17852
+      it('support re-insertion', () => {
+        const l = ['a', '*', '*', 'd', '-', '-', '-', 'e'];
+        differ.check(l);
+        l[1] = 'b';
+        l[5] = 'c';
+        differ.check(l);
+        expect(differ.toString()).toEqual(iterableChangesAsString({
+          collection: ['a', 'b[null->1]', '*[1->2]', 'd', '-', 'c[null->5]', '-[5->6]', 'e'],
+          previous: ['a', '*[1->2]', '*[2->null]', 'd', '-', '-[5->6]', '-[6->null]', 'e'],
+          additions: ['b[null->1]', 'c[null->5]'],
+          moves: ['*[1->2]', '-[5->6]'],
+          removals: ['*[2->null]', '-[6->null]'],
+        }));
+      });
+
       describe('forEachOperation', () => {
         function stringifyItemChange(record: any, p: number, c: number, originalIndex: number) {
           const suffix = originalIndex == null ? '' : ' [o=' + originalIndex + ']';
@@ -329,8 +332,8 @@ export function main() {
              const startData = [0, 1, 2, 3, 4, 5];
              const endData = [6, 2, 7, 0, 4, 8];
 
-             differ = differ.diff(startData);
+             differ = differ.diff(startData) !;
-             differ = differ.diff(endData);
+             differ = differ.diff(endData) !;
 
              const operations: string[] = [];
              differ.forEachOperation((item: any, prev: number, next: number) => {
@@ -352,12 +355,12 @@ export function main() {
              const startData = [0, 1, 2, 3];
              const endData = [2, 1];
 
-             differ = differ.diff(startData);
+             differ = differ.diff(startData) !;
-             differ = differ.diff(endData);
+             differ = differ.diff(endData) !;
 
              const operations: string[] = [];
              differ.forEachOperation((item: any, prev: number, next: number) => {
-               const value = modifyArrayUsingOperation(startData, endData, prev, next);
+               modifyArrayUsingOperation(startData, endData, prev, next);
                operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
              });
 
@@ -372,12 +375,12 @@ export function main() {
           const startData = [1, 2, 3, 4, 5, 6];
           const endData = [3, 6, 4, 9, 1, 2];
 
-          differ = differ.diff(startData);
+          differ = differ.diff(startData) !;
-          differ = differ.diff(endData);
+          differ = differ.diff(endData) !;
 
           const operations: string[] = [];
           differ.forEachOperation((item: any, prev: number, next: number) => {
-            const value = modifyArrayUsingOperation(startData, endData, prev, next);
+            modifyArrayUsingOperation(startData, endData, prev, next);
             operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
           });
 
@@ -393,12 +396,12 @@ export function main() {
           const startData = [0, 1, 2, 3, 4];
           const endData = [4, 1, 2, 3, 0, 5];
 
-          differ = differ.diff(startData);
+          differ = differ.diff(startData) !;
-          differ = differ.diff(endData);
+          differ = differ.diff(endData) !;
 
           const operations: string[] = [];
           differ.forEachOperation((item: any, prev: number, next: number) => {
-            const value = modifyArrayUsingOperation(startData, endData, prev, next);
+            modifyArrayUsingOperation(startData, endData, prev, next);
             operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
           });
 
@@ -414,12 +417,12 @@ export function main() {
           const startData = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11];
           const endData = [10, 11, 1, 5, 7, 8, 0, 5, 3, 6];
 
-          differ = differ.diff(startData);
+          differ = differ.diff(startData) !;
-          differ = differ.diff(endData);
+          differ = differ.diff(endData) !;
 
           const operations: string[] = [];
           differ.forEachOperation((item: any, prev: number, next: number) => {
-            const value = modifyArrayUsingOperation(startData, endData, prev, next);
+            modifyArrayUsingOperation(startData, endData, prev, next);
             operations.push(stringifyItemChange(item, prev, next, item.previousIndex));
           });
 
@@ -440,8 +443,8 @@ export function main() {
              const startData = [1, 2, 3, 4];
              const endData = [5, 6, 7, 8];
 
-             differ = differ.diff(startData);
+             differ = differ.diff(startData) !;
-             differ = differ.diff(endData);
+             differ = differ.diff(endData) !;
 
              const operations: string[] = [];
              differ.forEachOperation((item: any, prev: number, next: number) => {
@@ -465,7 +468,7 @@ export function main() {
 
         it('should treat null as an empty list', () => {
           differ.diff(['a', 'b']);
-          expect(differ.diff(null).toString()).toEqual(iterableChangesAsString({
+          expect(differ.diff(null !) !.toString()).toEqual(iterableChangesAsString({
             previous: ['a[0->null]', 'b[1->null]'],
             removals: ['a[0->null]', 'b[1->null]']
           }));
@@ -478,7 +481,7 @@ export function main() {
     });
 
     describe('trackBy function by id', function() {
-      let differ: any /** TODO #9100 */;
+      let differ: any;
 
       const trackByItemId = (index: number, item: any): any => item.id;
 
@@ -565,8 +568,9 @@ export function main() {
         }));
       });
     });
+
     describe('trackBy function by index', function() {
-      let differ: any /** TODO #9100 */;
+      let differ: DefaultIterableDiffer<string>;
 
       const trackByIndex = (index: number, item: any): number => index;
 
@@ -584,9 +588,6 @@ export function main() {
           identityChanges: ['h']
         }));
       });
-
     });
-
-
   });
 }

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

@@ -8,15 +8,14 @@
 
 import {ReflectiveInjector} from '@angular/core';
 import {IterableDiffers} from '@angular/core/src/change_detection/differs/iterable_differs';
-import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testing_internal';
 
 import {SpyIterableDifferFactory} from '../../spies';
 
 export function main() {
   describe('IterableDiffers', function() {
-    let factory1: any /** TODO #9100 */;
+    let factory1: any;
-    let factory2: any /** TODO #9100 */;
+    let factory2: any;
-    let factory3: any /** TODO #9100 */;
+    let factory3: any;
 
     beforeEach(() => {
       factory1 = new SpyIterableDifferFactory();
@@ -57,7 +56,7 @@ export function main() {
             .toThrowError(/Cannot extend IterableDiffers without a parent injector/);
       });
 
-      it('should extend di-inherited diffesr', () => {
+      it('should extend di-inherited differs', () => {
         const parent = new IterableDiffers([factory1]);
         const injector =
             ReflectiveInjector.resolveAndCreate([{provide: IterableDiffers, useValue: parent}]);