From c69fff15c9e001bd636b180758f1102d338564fb Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Thu, 6 Jul 2017 12:11:47 -0700 Subject: [PATCH] fix(core): fix re-insertions in the iterable differ (#17891) fixes #17852 --- .../differs/default_iterable_differ.ts | 35 ++++---- .../differs/iterable_differs.ts | 2 +- .../change_detector_util_spec.ts | 1 - .../differs/default_iterable_differ_spec.ts | 81 ++++++++++--------- .../differs/iterable_differs_spec.ts | 9 +-- 5 files changed, 64 insertions(+), 64 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 8c151aa2d4..6f5686b165 100644 --- a/packages/core/src/change_detection/differs/default_iterable_differ.ts +++ b/packages/core/src/change_detection/differs/default_iterable_differ.ts @@ -172,7 +172,6 @@ export class DefaultIterableDiffer implements IterableDiffer, IterableChan onDestroy() {} - // todo(vicb): optim for UnmodifiableListView (frozen arrays) check(collection: NgIterable): boolean { this._reset(); @@ -281,12 +280,12 @@ export class DefaultIterableDiffer implements IterableDiffer, IterableChan _mismatch(record: IterableChangeRecord_|null, item: V, itemTrackBy: any, index: number): IterableChangeRecord_ { // The previous record after which we will append the current one. - let previousRecord: IterableChangeRecord_; + let previousRecord: IterableChangeRecord_|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 implements IterableDiffer, IterableChan /** @internal */ _reinsertAfter( - record: IterableChangeRecord_, prevRecord: IterableChangeRecord_, + record: IterableChangeRecord_, prevRecord: IterableChangeRecord_|null, index: number): IterableChangeRecord_ { if (this._unlinkedRecords !== null) { this._unlinkedRecords.remove(record); @@ -419,8 +418,9 @@ export class DefaultIterableDiffer implements IterableDiffer, IterableChan } /** @internal */ - _moveAfter(record: IterableChangeRecord_, prevRecord: IterableChangeRecord_, index: number): - IterableChangeRecord_ { + _moveAfter( + record: IterableChangeRecord_, prevRecord: IterableChangeRecord_|null, + index: number): IterableChangeRecord_ { this._unlink(record); this._insertAfter(record, prevRecord, index); this._addToMoves(record, index); @@ -428,8 +428,9 @@ export class DefaultIterableDiffer implements IterableDiffer, IterableChan } /** @internal */ - _addAfter(record: IterableChangeRecord_, prevRecord: IterableChangeRecord_, index: number): - IterableChangeRecord_ { + _addAfter( + record: IterableChangeRecord_, prevRecord: IterableChangeRecord_|null, + index: number): IterableChangeRecord_ { this._insertAfter(record, prevRecord, index); if (this._additionsTail === null) { @@ -447,7 +448,7 @@ export class DefaultIterableDiffer implements IterableDiffer, IterableChan /** @internal */ _insertAfter( - record: IterableChangeRecord_, prevRecord: IterableChangeRecord_, + record: IterableChangeRecord_, prevRecord: IterableChangeRecord_|null, index: number): IterableChangeRecord_ { // todo(vicb) // assert(record != prevRecord); @@ -665,11 +666,11 @@ class _DuplicateItemRecordList { } // Returns a IterableChangeRecord_ having IterableChangeRecord_.trackById == trackById and - // IterableChangeRecord_.currentIndex >= afterIndex - get(trackById: any, afterIndex: number|null): IterableChangeRecord_|null { + // IterableChangeRecord_.currentIndex >= atOrAfterIndex + get(trackById: any, atOrAfterIndex: number|null): IterableChangeRecord_|null { let record: IterableChangeRecord_|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 { /** * 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_|null { + get(trackById: any, atOrAfterIndex: number|null): IterableChangeRecord_|null { const key = trackById; const recordList = this.map.get(key); - return recordList ? recordList.get(trackById, afterIndex) : null; + return recordList ? recordList.get(trackById, atOrAfterIndex) : null; } /** diff --git a/packages/core/src/change_detection/differs/iterable_differs.ts b/packages/core/src/change_detection/differs/iterable_differs.ts index 8d5b58a49a..72bad7296b 100644 --- a/packages/core/src/change_detection/differs/iterable_differs.ts +++ b/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 */ diff --git a/packages/core/test/change_detection/change_detector_util_spec.ts b/packages/core/test/change_detection/change_detector_util_spec.ts index a9ed81dfbd..d53569f9dd 100644 --- a/packages/core/test/change_detection/change_detector_util_spec.ts +++ b/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', () => { 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 862f67dfcd..a05137a07c 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 @@ -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; 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)', () => { - 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)', () => { + it('should ignore [NaN] != [NaN]', () => { 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(endData); + differ = differ.diff(startData) !; + 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(endData); + differ = differ.diff(startData) !; + 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(endData); + differ = differ.diff(startData) !; + 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(endData); + differ = differ.diff(startData) !; + 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(endData); + differ = differ.diff(startData) !; + 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(endData); + differ = differ.diff(startData) !; + 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; const trackByIndex = (index: number, item: any): number => index; @@ -584,9 +588,6 @@ export function main() { identityChanges: ['h'] })); }); - }); - - }); } diff --git a/packages/core/test/change_detection/differs/iterable_differs_spec.ts b/packages/core/test/change_detection/differs/iterable_differs_spec.ts index 24410f0249..8a94bb1799 100644 --- a/packages/core/test/change_detection/differs/iterable_differs_spec.ts +++ b/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 factory2: any /** TODO #9100 */; - let factory3: any /** TODO #9100 */; + let factory1: any; + let factory2: any; + 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}]);