Angularfire: Child changed causes entire list to be recreated

[echo-sean]

Not sure if I'm using AngularFire2 incorrectly, but I've noticed this issue.
(same thing happens if a child is added or removed)

Version info

Angular: 2.4.3
Firebase: 3.6.5
AngularFire: 2.0.0-beta.6

How to reproduce these conditions

Steps to set up and reproduce

• Create a list of "/items" in Firebase with a "name" property.
• Display the list and then try to update the name of 1 item (see code below)
• http://plnkr.co/edit/NCaX3xwE6PNSHyN2Cjn5?p=preview

Expected behavior

If a child is changed I expect only that item in the list to change.

Actual behavior

All components are removed and recreated.

Console (with 16 items)

list init
(16) item init
item clicked
(16) item destroyed
(16) item init

Code to recreate

import { Component, OnInit, OnDestroy, Input } from '@angular/core';
import { AngularFire, FirebaseListObservable } from 'angularfire2';

@Component({
    selector: 'my-list',
    template: '<ul><template ngFor let-item [ngForOf]="list | async" let-i="index"><my-item [index]="i" [data]="item"></my-item></template></ul>'
})

export class MyListComponent implements OnInit, OnDestroy {

    public list: FirebaseListObservable<any[]>;

    constructor(private af: AngularFire) { }

    public ngOnInit(): void {
        console.log('list init');
        this.list = this.af.database.list('items');
    }

    public ngOnDestroy(): void {
        console.log('list destroyed');
    }
}

@Component({
    selector: 'my-item',
    template: '<li (click)="changeName()">{{index}} {{data.name}}</li>'
})

export class MyItemComponent implements OnInit, OnDestroy {

    @Input('data') data: any;
    @Input('index') index: number;

    constructor(private af: AngularFire) { }

    public ngOnInit(): void {
        console.log('item init');
    }

    public ngOnDestroy(): void {
        console.log('item destroyed');
    }

    public changeName(): void {
        console.log('item clicked');
        this.af.database.object('').update({
            ['items/' + this.data.$key + '/name']: this.data.name + '' + Math.round(Math.random() * 9999)
        });
    }
}

[katowulf]

Looks like a dup of #574

[davideast]

Fixed in the latest release that went out today. Thank you for following the template too! It was helpful to know that you were on beta.6 :)

[echo-sean]

I just updated to 2.0.0-beta.7 and unfortunately it's still happening.

[WhatsThatItsPat]

I think you just need to use a trackBy function. Read this, especially starting with section "How does ngFor work when we add or remove elements from the list?", and thru the trackBy sections.

[echo-sean]

Thanks @patrickmcd, good point.
However adding trackBy results in each list item receiving a new data item (even though the data hasn't actually changed)
So it's definitely an improvement, but I still think this could be an issue in AngularFire2.

I'm not sure exactly what is going on in the source, but looks like this returns a new array:
https://github.com/angular/angularfire2/blob/master/src/database/firebase_list_factory.ts#L219

Updated code example below.
Added the trackBy and logging out ngOnChanges:

import { Component, OnInit, OnChanges, SimpleChanges, OnDestroy, Input } from '@angular/core';
import { AngularFire, FirebaseListObservable } from 'angularfire2';

@Component({
    selector: 'my-list',
    template: '<ul><template ngFor let-item [ngForOf]="list | async" let-i="index" [ngForTrackBy]="identify"><my-item [index]="i" [data]="item"></my-item></template></ul>'
})

export class MyListComponent implements OnInit, OnChanges, OnDestroy {

    public list: FirebaseListObservable<any[]>;

    constructor(private af: AngularFire) { }

    public ngOnInit(): void {
        console.log('list init');
        this.list = this.af.database.list('items');
    }

    public ngOnChanges(changes: SimpleChanges): void {
        console.log('list changed', changes);
    }

    public ngOnDestroy(): void {
        console.log('list destroyed');
    }

    public identify(index, item): string {
        return index + '-' + item.$key;
    }
}

@Component({
    selector: 'my-item',
    template: '<li (click)="changeName()">{{index}} {{data.name}}</li>'
})

export class MyItemComponent implements OnInit, OnChanges, OnDestroy {

    @Input('data') data: any;
    @Input('index') index: number;

    constructor(private af: AngularFire) { }

    public ngOnInit(): void {
        console.log('item init');
    }

    public ngOnChanges(changes: SimpleChanges): void {
        console.log('item changed', changes);
    }

    public ngOnDestroy(): void {
        console.log('item destroyed');
    }

    public changeName(): void {
        console.log('item clicked');
        this.af.database.object('').update({
            ['items/' + this.data.$key + '/name']: this.data.name + '' + Math.round(Math.random() * 9999)
        });
    }
}

[WhatsThatItsPat]

Your trackBy function may be returning too much. Because indexes change when the data changes, it's defeating the purpose of keeping track of each item. Just return the object key.

public identify(index, item): string {
  // return index + '-' + item.$key;
  return item.$key;
}

Or, here's a pithier way to write it.

trackFbObjects = (idx, obj) => obj.$key;

[echo-sean]

Thanks for the info @patrickmcd, that makes sense.
Changing to use just "return item.$key;" still has the same issue though.

[illambo]

I tested a similar example, for me with _trackBy_ works well (dom childs are not destroy), however if you inject item (input) in child component _ngOnChanges_ of Item component are always triggered (maybe for comparison ===).
I have found a good example for this here, seems is it possible implement _DoCheck_ method.
Here another good example with _IterableDiffer_ usefull for debug purpose.
Maybe it can be useful

[echo-sean]

Here's a quick demo of the issue:
http://plnkr.co/edit/NCaX3xwE6PNSHyN2Cjn5?p=preview

[illambo]

Hi, I tested your example and only the item clicked are recreated ( with destroy ), not the entire list (I verified destroy log on console).
You also have the same result ?

[echo-sean]

Sorry, I didn't explain what the example is showing.
In both lists, when an item changes (or is recreated) the background colour will change.
So you can see which components are being redrawn.

The top list is using AngularFire2 with "trackBy".
Result is that when 1 item changes all items in the list get "ngOnChanges".

The bottom list is using firebase directly (albeit a quickly hacked together example).
Result is that the item that has changed gets removed and added again.

Both are not perfect.
Ideally only the item that has changed should get ngOnChanges called.

[illambo]

_ngOnChanges_ is triggered for all the childs caused by the _===_ comparison,
i think you need implement your custom logic for check the really changes like this :
public ngOnChanges(changes: {[propName: string]: SimpleChange}): void { if ( (changes['data'].currentValue.name != changes['data'].previousValue.name)) { console.log(this.tt, 'item really changed', changes); this.numChanges++; this.colour = this.getRandomColour(); } }
or you can use something like loadash for deep object comparison

[echo-sean]

Agreed, although my point is that I shouldn't need to.
I shouldn't have to know that I need to use trackBy either. Perhaps this should be in the docs?

[louisptremblay]

This is very suboptimal when you have a lot of items... if you have 20000 svg points and one moves, you need to call ngOnChanges (deepEqual) 20000 times. Instead, AngularFirestoreCollection.stateChanges() 'modified' object has all the info to be able to call it only once on the targeted object.

EDIT: seems possible to have ngOnChanges trigger only on the modified object(s) by binding the ngFor to a Map(). something like:

public points: Map = new Map();
db.collection<Point>(DB_PATH).stateChanges()
  .subscribe(changes => {
      changes.forEach(change => {
        const pt = change.payload.doc.data() as Point;
        switch (change.type) {
          case 'added':
          case 'modified':
            this.points.set(pt.id, pt);
            break;
          case 'removed':
            this.points.delete(pt.id);
            break;
        }
      });
    });

You also need a separate getter for map keys, because ngFor refuses to work with MapIterable
https://github.com/angular/angular/issues/2246#issuecomment-341763687