X Tutup
Skip to content

Commit 0f10624

Browse files
karakara
committed
fix(ngFor): update view locals if identity changes
Closes #6923
1 parent 6f1ef33 commit 0f10624

File tree

6 files changed

+147
-38
lines changed

6 files changed

+147
-38
lines changed

‎modules/angular2/src/common/directives/ng_for.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ export class NgFor implements DoCheck {
117117
var viewRef = <EmbeddedViewRef>this._viewContainer.get(i);
118118
viewRef.setLocal('last', i === ilen - 1);
119119
}
120+
121+
changes.forEachIdentityChange((record) => {
122+
var viewRef = <EmbeddedViewRef>this._viewContainer.get(record.currentIndex);
123+
viewRef.setLocal('\$implicit', record.item);
124+
});
120125
}
121126

122127
private _perViewChange(view, record) {

‎modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts‎

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ export class DefaultIterableDiffer implements IterableDiffer {
4040
private _movesTail: CollectionChangeRecord = null;
4141
private _removalsHead: CollectionChangeRecord = null;
4242
private _removalsTail: CollectionChangeRecord = null;
43+
// Keeps track of records where custom track by is the same, but item identity has changed
44+
private _identityChangesHead: CollectionChangeRecord = null;
45+
private _identityChangesTail: CollectionChangeRecord = null;
4346

4447
constructor(private _trackByFn?: TrackByFn) {
4548
this._trackByFn = isPresent(this._trackByFn) ? this._trackByFn : trackByIdentity;
@@ -84,6 +87,13 @@ export class DefaultIterableDiffer implements IterableDiffer {
8487
}
8588
}
8689

90+
forEachIdentityChange(fn: Function) {
91+
var record: CollectionChangeRecord;
92+
for (record = this._identityChangesHead; record !== null; record = record._nextIdentityChange) {
93+
fn(record);
94+
}
95+
}
96+
8797
diff(collection: any): DefaultIterableDiffer {
8898
if (isBlank(collection)) collection = [];
8999
if (!isListLikeIterable(collection)) {
@@ -123,7 +133,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
123133
// TODO(misko): can we limit this to duplicates only?
124134
record = this._verifyReinsertion(record, item, itemTrackBy, index);
125135
}
126-
record.item = item;
136+
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
127137
}
128138

129139
record = record._next;
@@ -135,9 +145,12 @@ export class DefaultIterableDiffer implements IterableDiffer {
135145
if (record === null || !looseIdentical(record.trackById, itemTrackBy)) {
136146
record = this._mismatch(record, item, itemTrackBy, index);
137147
mayBeDirty = true;
138-
} else if (mayBeDirty) {
139-
// TODO(misko): can we limit this to duplicates only?
140-
record = this._verifyReinsertion(record, item, itemTrackBy, index);
148+
} else {
149+
if (mayBeDirty) {
150+
// TODO(misko): can we limit this to duplicates only?
151+
record = this._verifyReinsertion(record, item, itemTrackBy, index);
152+
}
153+
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
141154
}
142155
record = record._next;
143156
index++;
@@ -150,9 +163,12 @@ export class DefaultIterableDiffer implements IterableDiffer {
150163
return this.isDirty;
151164
}
152165

153-
// CollectionChanges is considered dirty if it has any additions, moves or removals.
166+
/* CollectionChanges is considered dirty if it has any additions, moves, removals, or identity
167+
* changes.
168+
*/
154169
get isDirty(): boolean {
155-
return this._additionsHead !== null || this._movesHead !== null || this._removalsHead !== null;
170+
return this._additionsHead !== null || this._movesHead !== null ||
171+
this._removalsHead !== null || this._identityChangesHead !== null;
156172
}
157173

158174
/**
@@ -183,6 +199,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
183199
}
184200
this._movesHead = this._movesTail = null;
185201
this._removalsHead = this._removalsTail = null;
202+
this._identityChangesHead = this._identityChangesTail = null;
186203

187204
// todo(vicb) when assert gets supported
188205
// assert(!this.isDirty);
@@ -216,12 +233,18 @@ export class DefaultIterableDiffer implements IterableDiffer {
216233
record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
217234
if (record !== null) {
218235
// We have seen this before, we need to move it forward in the collection.
236+
// But first we need to check if identity changed, so we can update in view if necessary
237+
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
238+
219239
this._moveAfter(record, previousRecord, index);
220240
} else {
221241
// Never seen it, check evicted list.
222242
record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy);
223243
if (record !== null) {
224244
// It is an item which we have evicted earlier: reinsert it back into the list.
245+
// But first we need to check if identity changed, so we can update in view if necessary
246+
if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
247+
225248
this._reinsertAfter(record, previousRecord, index);
226249
} else {
227250
// It is a new item: add it.
@@ -269,7 +292,6 @@ export class DefaultIterableDiffer implements IterableDiffer {
269292
record.currentIndex = index;
270293
this._addToMoves(record, index);
271294
}
272-
record.item = item;
273295
return record;
274296
}
275297

@@ -469,6 +491,18 @@ export class DefaultIterableDiffer implements IterableDiffer {
469491
return record;
470492
}
471493

494+
/** @internal */
495+
_addIdentityChange(record: CollectionChangeRecord, item: any) {
496+
record.item = item;
497+
if (this._identityChangesTail === null) {
498+
this._identityChangesTail = this._identityChangesHead = record;
499+
} else {
500+
this._identityChangesTail = this._identityChangesTail._nextIdentityChange = record;
501+
}
502+
return record;
503+
}
504+
505+
472506
toString(): string {
473507
var list = [];
474508
this.forEachItem((record) => list.push(record));
@@ -485,9 +519,13 @@ export class DefaultIterableDiffer implements IterableDiffer {
485519
var removals = [];
486520
this.forEachRemovedItem((record) => removals.push(record));
487521

522+
var identityChanges = [];
523+
this.forEachIdentityChange((record) => identityChanges.push(record));
524+
488525
return "collection: " + list.join(', ') + "\n" + "previous: " + previous.join(', ') + "\n" +
489526
"additions: " + additions.join(', ') + "\n" + "moves: " + moves.join(', ') + "\n" +
490-
"removals: " + removals.join(', ') + "\n";
527+
"removals: " + removals.join(', ') + "\n" + "identityChanges: " +
528+
identityChanges.join(', ') + "\n";
491529
}
492530
}
493531

@@ -513,6 +551,9 @@ export class CollectionChangeRecord {
513551
_nextAdded: CollectionChangeRecord = null;
514552
/** @internal */
515553
_nextMoved: CollectionChangeRecord = null;
554+
/** @internal */
555+
_nextIdentityChange: CollectionChangeRecord = null;
556+
516557

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

‎modules/angular2/test/common/directives/ng_for_spec.ts‎

Lines changed: 56 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -361,30 +361,64 @@ export function main() {
361361
});
362362
}));
363363

364-
it('should use custom track by if function is provided',
365-
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
366-
var template =
367-
`<template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy" #i="index">
364+
describe('track by', function() {
365+
it('should not replace tracked items',
366+
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
367+
var template =
368+
`<template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy" #i="index">
368369
<p>{{items[i]}}</p>
369370
</template>`;
370-
tcb.overrideTemplate(TestComponent, template)
371-
.createAsync(TestComponent)
372-
.then((fixture) => {
373-
var buildItemList =
374-
() => {
375-
fixture.debugElement.componentInstance.items = [{'id': 'a'}];
376-
fixture.detectChanges();
377-
return fixture.debugElement.queryAll(By.css('p'))[0];
378-
}
379-
380-
var firstP = buildItemList();
381-
var finalP = buildItemList();
382-
expect(finalP.nativeElement).toBe(firstP.nativeElement);
383-
async.done();
384-
});
385-
}));
386-
387-
371+
tcb.overrideTemplate(TestComponent, template)
372+
.createAsync(TestComponent)
373+
.then((fixture) => {
374+
var buildItemList =
375+
() => {
376+
fixture.debugElement.componentInstance.items = [{'id': 'a'}];
377+
fixture.detectChanges();
378+
return fixture.debugElement.queryAll(By.css('p'))[0];
379+
}
380+
381+
var firstP = buildItemList();
382+
var finalP = buildItemList();
383+
expect(finalP.nativeElement).toBe(firstP.nativeElement);
384+
async.done();
385+
});
386+
}));
387+
it('should update implicit local variable on view',
388+
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
389+
var template =
390+
`<div><template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy">{{item['color']}}</template></div>`;
391+
tcb.overrideTemplate(TestComponent, template)
392+
.createAsync(TestComponent)
393+
.then((fixture) => {
394+
fixture.debugElement.componentInstance.items = [{'id': 'a', 'color': 'blue'}];
395+
fixture.detectChanges();
396+
expect(fixture.debugElement.nativeElement).toHaveText('blue');
397+
fixture.debugElement.componentInstance.items = [{'id': 'a', 'color': 'red'}];
398+
fixture.detectChanges();
399+
expect(fixture.debugElement.nativeElement).toHaveText('red');
400+
async.done();
401+
});
402+
}));
403+
it('should move items around and keep them updated ',
404+
inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
405+
var template =
406+
`<div><template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy">{{item['color']}}</template></div>`;
407+
tcb.overrideTemplate(TestComponent, template)
408+
.createAsync(TestComponent)
409+
.then((fixture) => {
410+
fixture.debugElement.componentInstance.items =
411+
[{'id': 'a', 'color': 'blue'}, {'id': 'b', 'color': 'yellow'}];
412+
fixture.detectChanges();
413+
expect(fixture.debugElement.nativeElement).toHaveText('blueyellow');
414+
fixture.debugElement.componentInstance.items =
415+
[{'id': 'b', 'color': 'orange'}, {'id': 'a', 'color': 'red'}];
416+
fixture.detectChanges();
417+
expect(fixture.debugElement.nativeElement).toHaveText('orangered');
418+
async.done();
419+
});
420+
}));
421+
});
388422
});
389423
}
390424

‎modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ class ItemWithId {
2525
toString() { return `{id: ${this.id}}` }
2626
}
2727

28+
class ComplexItem {
29+
constructor(private id: string, private color: string) {}
30+
31+
toString() { return `{id: ${this.id}, color: ${this.color}}` }
32+
}
33+
2834
// todo(vicb): UnmodifiableListView / frozen object when implemented
2935
export function main() {
3036
describe('iterable differ', function() {
@@ -342,8 +348,12 @@ export function main() {
342348

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

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

356+
it('should treat seen records as identity changes, not additions', () => {
347357
let l = buildItemList(['a', 'b', 'c']);
348358
differ.check(l);
349359
expect(differ.toString())
@@ -357,11 +367,26 @@ export function main() {
357367
expect(differ.toString())
358368
.toEqual(iterableChangesAsString({
359369
collection: [`{id: a}`, `{id: b}`, `{id: c}`],
370+
identityChanges: [`{id: a}`, `{id: b}`, `{id: c}`],
360371
previous: [`{id: a}`, `{id: b}`, `{id: c}`]
361372
}));
362373
});
363374

364-
it('should track moves normally with track by function', () => {
375+
it('should have updated properties in identity change collection', () => {
376+
let l = [new ComplexItem('a', 'blue'), new ComplexItem('b', 'yellow')];
377+
differ.check(l);
378+
379+
l = [new ComplexItem('a', 'orange'), new ComplexItem('b', 'red')];
380+
differ.check(l);
381+
expect(differ.toString())
382+
.toEqual(iterableChangesAsString({
383+
collection: [`{id: a, color: orange}`, `{id: b, color: red}`],
384+
identityChanges: [`{id: a, color: orange}`, `{id: b, color: red}`],
385+
previous: [`{id: a, color: orange}`, `{id: b, color: red}`]
386+
}));
387+
});
388+
389+
it('should track moves normally', () => {
365390
let l = buildItemList(['a', 'b', 'c']);
366391
differ.check(l);
367392

@@ -370,13 +395,14 @@ export function main() {
370395
expect(differ.toString())
371396
.toEqual(iterableChangesAsString({
372397
collection: ['{id: b}[1->0]', '{id: a}[0->1]', '{id: c}'],
398+
identityChanges: ['{id: b}[1->0]', '{id: a}[0->1]', '{id: c}'],
373399
previous: ['{id: a}[0->1]', '{id: b}[1->0]', '{id: c}'],
374400
moves: ['{id: b}[1->0]', '{id: a}[0->1]']
375401
}));
376402

377403
});
378404

379-
it('should track duplicate reinsertion normally with track by function', () => {
405+
it('should track duplicate reinsertion normally', () => {
380406
let l = buildItemList(['a', 'a']);
381407
differ.check(l);
382408

@@ -385,14 +411,15 @@ export function main() {
385411
expect(differ.toString())
386412
.toEqual(iterableChangesAsString({
387413
collection: ['{id: b}[null->0]', '{id: a}[0->1]', '{id: a}[1->2]'],
414+
identityChanges: ['{id: a}[0->1]', '{id: a}[1->2]'],
388415
previous: ['{id: a}[0->1]', '{id: a}[1->2]'],
389416
moves: ['{id: a}[0->1]', '{id: a}[1->2]'],
390417
additions: ['{id: b}[null->0]']
391418
}));
392419

393420
});
394421

395-
it('should track removals normally with track by function', () => {
422+
it('should track removals normally', () => {
396423
let l = buildItemList(['a', 'b', 'c']);
397424
differ.check(l);
398425

‎modules/angular2/test/core/change_detection/util.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import {isBlank, CONST_EXPR} from 'angular2/src/facade/lang';
22

3-
export function iterableChangesAsString({collection = CONST_EXPR([]), previous = CONST_EXPR([]),
4-
additions = CONST_EXPR([]), moves = CONST_EXPR([]),
5-
removals = CONST_EXPR([])}) {
3+
export function iterableChangesAsString(
4+
{collection = CONST_EXPR([]), previous = CONST_EXPR([]), additions = CONST_EXPR([]),
5+
moves = CONST_EXPR([]), removals = CONST_EXPR([]), identityChanges = CONST_EXPR([])}) {
66
return "collection: " + collection.join(', ') + "\n" + "previous: " + previous.join(', ') + "\n" +
77
"additions: " + additions.join(', ') + "\n" + "moves: " + moves.join(', ') + "\n" +
8-
"removals: " + removals.join(', ') + "\n";
8+
"removals: " + removals.join(', ') + "\n" + "identityChanges: " +
9+
identityChanges.join(', ') + "\n";
910
}
1011

1112
export function kvChangesAsString(

‎modules/angular2/test/public_api_spec.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,7 @@ var NG_COMMON = [
419419
'ObservableListDiff.forEachMovedItem():dart',
420420
'ObservableListDiff.forEachPreviousItem():dart',
421421
'ObservableListDiff.forEachRemovedItem():dart',
422+
'ObservableListDiff.forEachIdentityChange():dart',
422423
'ObservableListDiff.isDirty:dart',
423424
'ObservableListDiff.length:dart',
424425
'ObservableListDiff.onDestroy():dart',

0 commit comments

Comments
 (0)
X Tutup