Skip to content

fix(ngFor): update view values manually with track by #6923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kara wants to merge 1 commit into from
Closed

fix(ngFor): update view values manually with track by #6923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions modules/angular2/src/common/directives/ng_for.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ export class NgFor implements DoCheck {
var viewRef = <EmbeddedViewRef>this._viewContainer.get(i);
viewRef.setLocal('last', i === ilen - 1);
}

changes.forEachIdentityChange((record) => {
var viewRef = <EmbeddedViewRef>this._viewContainer.get(record.currentIndex);
viewRef.setLocal('\$implicit', record.item);
});
}

private _perViewChange(view, record) {
Expand Down
57 changes: 49 additions & 8 deletions modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ export class DefaultIterableDiffer implements IterableDiffer {
private _movesTail: CollectionChangeRecord = null;
private _removalsHead: CollectionChangeRecord = null;
private _removalsTail: CollectionChangeRecord = null;
// Keeps track of records where custom track by is the same, but item identity has changed
private _identityChangesHead: CollectionChangeRecord = null;
private _identityChangesTail: CollectionChangeRecord = null;

constructor(private _trackByFn?: TrackByFn) {
this._trackByFn = isPresent(this._trackByFn) ? this._trackByFn : trackByIdentity;
Expand Down Expand Up @@ -84,6 +87,13 @@ export class DefaultIterableDiffer implements IterableDiffer {
}
}

forEachIdentityChange(fn: Function) {
var record: CollectionChangeRecord;
for (record = this._identityChangesHead; record !== null; record = record._nextIdentityChange) {
fn(record);
}
}

diff(collection: any): DefaultIterableDiffer {
if (isBlank(collection)) collection = [];
if (!isListLikeIterable(collection)) {
Expand Down Expand Up @@ -123,7 +133,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
// TODO(misko): can we limit this to duplicates only?
record = this._verifyReinsertion(record, item, itemTrackBy, index);
}
record.item = item;
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
}

record = record._next;
Expand All @@ -135,9 +145,12 @@ export class DefaultIterableDiffer implements IterableDiffer {
if (record === null || !looseIdentical(record.trackById, itemTrackBy)) {
record = this._mismatch(record, item, itemTrackBy, index);
mayBeDirty = true;
} else if (mayBeDirty) {
// TODO(misko): can we limit this to duplicates only?
record = this._verifyReinsertion(record, item, itemTrackBy, index);
} else {
if (mayBeDirty) {
// TODO(misko): can we limit this to duplicates only?
record = this._verifyReinsertion(record, item, itemTrackBy, index);
}
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
}
record = record._next;
index++;
Expand All @@ -150,9 +163,12 @@ export class DefaultIterableDiffer implements IterableDiffer {
return this.isDirty;
}

// CollectionChanges is considered dirty if it has any additions, moves or removals.
/* CollectionChanges is considered dirty if it has any additions, moves, removals, or identity
* changes.
*/
get isDirty(): boolean {
return this._additionsHead !== null || this._movesHead !== null || this._removalsHead !== null;
return this._additionsHead !== null || this._movesHead !== null ||
this._removalsHead !== null || this._identityChangesHead !== null;
}

/**
Expand Down Expand Up @@ -183,6 +199,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
}
this._movesHead = this._movesTail = null;
this._removalsHead = this._removalsTail = null;
this._identityChangesHead = this._identityChangesTail = null;

// todo(vicb) when assert gets supported
// assert(!this.isDirty);
Expand Down Expand Up @@ -216,12 +233,18 @@ export class DefaultIterableDiffer implements IterableDiffer {
record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
if (record !== null) {
// We have seen this before, we need to move it forward in the collection.
// But first we need to check if identity changed, so we can update in view if necessary
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);

this._moveAfter(record, previousRecord, index);
} else {
// Never seen it, check evicted list.
record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy);
if (record !== null) {
// It is an item which we have evicted earlier: reinsert it back into the list.
// But first we need to check if identity changed, so we can update in view if necessary
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);

this._reinsertAfter(record, previousRecord, index);
} else {
// It is a new item: add it.
Expand Down Expand Up @@ -269,7 +292,6 @@ export class DefaultIterableDiffer implements IterableDiffer {
record.currentIndex = index;
this._addToMoves(record, index);
}
record.item = item;
return record;
}

Expand Down Expand Up @@ -469,6 +491,18 @@ export class DefaultIterableDiffer implements IterableDiffer {
return record;
}

/** @internal */
_addIdentityChange(record: CollectionChangeRecord, item: any) {
record.item = item;
if (this._identityChangesTail === null) {
this._identityChangesTail = this._identityChangesHead = record;
} else {
this._identityChangesTail = this._identityChangesTail._nextIdentityChange = record;
}
return record;
}


toString(): string {
var list = [];
this.forEachItem((record) => list.push(record));
Expand All @@ -485,9 +519,13 @@ export class DefaultIterableDiffer implements IterableDiffer {
var removals = [];
this.forEachRemovedItem((record) => removals.push(record));

var identityChanges = [];
this.forEachIdentityChange((record) => identityChanges.push(record));

return "collection: " + list.join(', ') + "\n" + "previous: " + previous.join(', ') + "\n" +
"additions: " + additions.join(', ') + "\n" + "moves: " + moves.join(', ') + "\n" +
"removals: " + removals.join(', ') + "\n";
"removals: " + removals.join(', ') + "\n" + "identityChanges: " +
identityChanges.join(', ') + "\n";
}
}

Expand All @@ -513,6 +551,9 @@ export class CollectionChangeRecord {
_nextAdded: CollectionChangeRecord = null;
/** @internal */
_nextMoved: CollectionChangeRecord = null;
/** @internal */
_nextIdentityChange: CollectionChangeRecord = null;


constructor(public item: any, public trackById: any) {}

Expand Down
78 changes: 56 additions & 22 deletions modules/angular2/test/common/directives/ng_for_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,30 +361,64 @@ export function main() {
});
}));

it('should use custom track by if function is provided',
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
var template =
`<template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy" #i="index">
describe('track by', function() {
it('should not replace tracked items',
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
var template =
`<template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy" #i="index">
<p>{{items[i]}}</p>
</template>`;
tcb.overrideTemplate(TestComponent, template)
.createAsync(TestComponent)
.then((fixture) => {
var buildItemList =
() => {
fixture.debugElement.componentInstance.items = [{'id': 'a'}];
fixture.detectChanges();
return fixture.debugElement.queryAll(By.css('p'))[0];
}

var firstP = buildItemList();
var finalP = buildItemList();
expect(finalP.nativeElement).toBe(firstP.nativeElement);
async.done();
});
}));


tcb.overrideTemplate(TestComponent, template)
.createAsync(TestComponent)
.then((fixture) => {
var buildItemList =
() => {
fixture.debugElement.componentInstance.items = [{'id': 'a'}];
fixture.detectChanges();
return fixture.debugElement.queryAll(By.css('p'))[0];
}

var firstP = buildItemList();
var finalP = buildItemList();
expect(finalP.nativeElement).toBe(firstP.nativeElement);
async.done();
});
}));
it('should update implicit local variable on view',
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
var template =
`<div><template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy">{{item['color']}}</template></div>`;
tcb.overrideTemplate(TestComponent, template)
.createAsync(TestComponent)
.then((fixture) => {
fixture.debugElement.componentInstance.items = [{'id': 'a', 'color': 'blue'}];
fixture.detectChanges();
expect(fixture.debugElement.nativeElement).toHaveText('blue');
fixture.debugElement.componentInstance.items = [{'id': 'a', 'color': 'red'}];
fixture.detectChanges();
expect(fixture.debugElement.nativeElement).toHaveText('red');
async.done();
});
}));
it('should move items around and keep them updated ',
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
var template =
`<div><template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy">{{item['color']}}</template></div>`;
tcb.overrideTemplate(TestComponent, template)
.createAsync(TestComponent)
.then((fixture) => {
fixture.debugElement.componentInstance.items =
[{'id': 'a', 'color': 'blue'}, {'id': 'b', 'color': 'yellow'}];
fixture.detectChanges();
expect(fixture.debugElement.nativeElement).toHaveText('blueyellow');
fixture.debugElement.componentInstance.items =
[{'id': 'b', 'color': 'orange'}, {'id': 'a', 'color': 'red'}];
fixture.detectChanges();
expect(fixture.debugElement.nativeElement).toHaveText('orangered');
async.done();
});
}));
});
});
}

Expand Down
35 changes: 31 additions & 4 deletions modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ class ItemWithId {
toString() { return `{id: ${this.id}}` }
}

class ComplexItem {
constructor(private id: string, private color: string) {}

toString() { return `{id: ${this.id}, color: ${this.color}}` }
}

// todo(vicb): UnmodifiableListView / frozen object when implemented
export function main() {
describe('iterable differ', function() {
Expand Down Expand Up @@ -342,8 +348,12 @@ export function main() {

beforeEach(() => { differ = new DefaultIterableDiffer(trackByItemId); });

it('should not treat maps as new with track by function', () => {
it('should treat the collection as dirty if identity changes', () => {
differ.diff(buildItemList(['a']));
expect(differ.diff(buildItemList(['a']))).toBe(differ);
});

it('should treat seen records as identity changes, not additions', () => {
let l = buildItemList(['a', 'b', 'c']);
differ.check(l);
expect(differ.toString())
Expand All @@ -357,11 +367,26 @@ export function main() {
expect(differ.toString())
.toEqual(iterableChangesAsString({
collection: [`{id: a}`, `{id: b}`, `{id: c}`],
identityChanges: [`{id: a}`, `{id: b}`, `{id: c}`],
previous: [`{id: a}`, `{id: b}`, `{id: c}`]
}));
});

it('should track moves normally with track by function', () => {
it('should have updated properties in identity change collection', () => {
let l = [new ComplexItem('a', 'blue'), new ComplexItem('b', 'yellow')];
differ.check(l);

l = [new ComplexItem('a', 'orange'), new ComplexItem('b', 'red')];
differ.check(l);
expect(differ.toString())
.toEqual(iterableChangesAsString({
collection: [`{id: a, color: orange}`, `{id: b, color: red}`],
identityChanges: [`{id: a, color: orange}`, `{id: b, color: red}`],
previous: [`{id: a, color: orange}`, `{id: b, color: red}`]
}));
});

it('should track moves normally', () => {
let l = buildItemList(['a', 'b', 'c']);
differ.check(l);

Expand All @@ -370,13 +395,14 @@ export function main() {
expect(differ.toString())
.toEqual(iterableChangesAsString({
collection: ['{id: b}[1->0]', '{id: a}[0->1]', '{id: c}'],
identityChanges: ['{id: b}[1->0]', '{id: a}[0->1]', '{id: c}'],
previous: ['{id: a}[0->1]', '{id: b}[1->0]', '{id: c}'],
moves: ['{id: b}[1->0]', '{id: a}[0->1]']
}));

});

it('should track duplicate reinsertion normally with track by function', () => {
it('should track duplicate reinsertion normally', () => {
let l = buildItemList(['a', 'a']);
differ.check(l);

Expand All @@ -385,14 +411,15 @@ export function main() {
expect(differ.toString())
.toEqual(iterableChangesAsString({
collection: ['{id: b}[null->0]', '{id: a}[0->1]', '{id: a}[1->2]'],
identityChanges: ['{id: a}[0->1]', '{id: a}[1->2]'],
previous: ['{id: a}[0->1]', '{id: a}[1->2]'],
moves: ['{id: a}[0->1]', '{id: a}[1->2]'],
additions: ['{id: b}[null->0]']
}));

});

it('should track removals normally with track by function', () => {
it('should track removals normally', () => {
let l = buildItemList(['a', 'b', 'c']);
differ.check(l);

Expand Down
9 changes: 5 additions & 4 deletions modules/angular2/test/core/change_detection/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import {isBlank, CONST_EXPR} from 'angular2/src/facade/lang';

export function iterableChangesAsString({collection = CONST_EXPR([]), previous = CONST_EXPR([]),
additions = CONST_EXPR([]), moves = CONST_EXPR([]),
removals = CONST_EXPR([])}) {
export function iterableChangesAsString(
{collection = CONST_EXPR([]), previous = CONST_EXPR([]), additions = CONST_EXPR([]),
moves = CONST_EXPR([]), removals = CONST_EXPR([]), identityChanges = CONST_EXPR([])}) {
return "collection: " + collection.join(', ') + "\n" + "previous: " + previous.join(', ') + "\n" +
"additions: " + additions.join(', ') + "\n" + "moves: " + moves.join(', ') + "\n" +
"removals: " + removals.join(', ') + "\n";
"removals: " + removals.join(', ') + "\n" + "identityChanges: " +
identityChanges.join(', ') + "\n";
}

export function kvChangesAsString(
Expand Down
1 change: 1 addition & 0 deletions modules/angular2/test/public_api_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ var NG_COMMON = [
'ObservableListDiff.forEachMovedItem():dart',
'ObservableListDiff.forEachPreviousItem():dart',
'ObservableListDiff.forEachRemovedItem():dart',
'ObservableListDiff.forEachIdentityChange():dart',
'ObservableListDiff.isDirty:dart',
'ObservableListDiff.length:dart',
'ObservableListDiff.onDestroy():dart',
Expand Down