Skip to content

Commit

Permalink
fix(core): fix re-insertions in the iterable differ (angular#17891)
Browse files Browse the repository at this point in the history 
fixes angular#17852
  • Loading branch information
@vicb
vicb authored and Zhicheng Wang committed Aug 11, 2017
1 parent 1e172d2 commit 893c08b
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 64 deletions.
35 changes: 18 additions & 17 deletions packages/core/src/change_detection/differs/default_iterable_differ.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -419,17 +418,19 @@ export class DefaultIterableDiffer<V> implements IterableDiffer<V>, IterableChan
}

/** @internal */
_moveAfter(record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>, index: number):
IterableChangeRecord_<V> {
_moveAfter(
record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>|null,
index: number): IterableChangeRecord_<V> {
this._unlink(record);
this._insertAfter(record, prevRecord, index);
this._addToMoves(record, index);
return record;
}

/** @internal */
_addAfter(record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>, index: number):
IterableChangeRecord_<V> {
_addAfter(
record: IterableChangeRecord_<V>, prevRecord: IterableChangeRecord_<V>|null,
index: number): IterableChangeRecord_<V> {
this._insertAfter(record, prevRecord, index);

if (this._additionsTail === null) {
Expand All @@ -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);
Expand Down Expand Up @@ -665,11 +666,11 @@ class _DuplicateItemRecordList<V> {
}

// Returns a IterableChangeRecord_ having IterableChangeRecord_.trackById == trackById and
// IterableChangeRecord_.currentIndex >= afterIndex
get(trackById: any, afterIndex: number|null): IterableChangeRecord_<V>|null {
// IterableChangeRecord_.currentIndex >= atOrAfterIndex
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;
}
Expand Down Expand Up @@ -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;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/change_detection/differs/iterable_differs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
1 change: 0 additions & 1 deletion packages/core/test/change_detection/change_detector_util_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
81 changes: 41 additions & 40 deletions packages/core/test/change_detection/differs/default_iterable_differ_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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(); });

Expand All @@ -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: []}));
Expand All @@ -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: []}));

Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 + ']';
Expand Down Expand Up @@ -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) => {
Expand All @@ -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));
});

Expand All @@ -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));
});

Expand All @@ -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));
});

Expand All @@ -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));
});

Expand All @@ -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) => {
Expand All @@ -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]']
}));
Expand All @@ -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;

Expand Down Expand Up @@ -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;

Expand All @@ -584,9 +588,6 @@ export function main() {
identityChanges: ['h']
}));
});

});


});
}
9 changes: 4 additions & 5 deletions packages/core/test/change_detection/differs/iterable_differs_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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}]);
Expand Down

0 comments on commit 893c08b

Please sign in to comment.