ngFor + ngModel: How can I unshift values to the array I am iterating?

Question

I have an array of elements which the user can not only edit, but also add and delete complete array elements. This works nicely, unless I attempt to add a value to the beginning of the array (e.g. using unshift).

Here is a test demonstrating my problem:

import { Component } from '@angular/core';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { FormsModule } from '@angular/forms';


@Component({
    template: `
        <form>
            <div *ngFor="let item of values; let index = index">
                <input [name]="'elem' + index" [(ngModel)]="item.value">
            </div>
        </form>`
})
class TestComponent {
    values: {value: string}[] = [{value: 'a'}, {value: 'b'}];
}

fdescribe('ngFor/Model', () => {
    let component: TestComponent;
    let fixture: ComponentFixture<TestComponent>;
    let element: HTMLDivElement;

    beforeEach(async () => {
        TestBed.configureTestingModule({
            imports: [FormsModule],
            declarations: [TestComponent]
        });

        fixture = TestBed.createComponent(TestComponent);
        component = fixture.componentInstance;
        element = fixture.nativeElement;

        fixture.detectChanges();
        await fixture.whenStable();
    });

    function getAllValues() {
        return Array.from(element.querySelectorAll('input')).map(elem => elem.value);
    }

    it('should display all values', async () => {
        // evaluation
        expect(getAllValues()).toEqual(['a', 'b']);
    });

    it('should display all values after push', async () => {
        // execution
        component.values.push({value: 'c'});
        fixture.detectChanges();
        await fixture.whenStable();

        // evaluation
        expect(getAllValues()).toEqual(['a', 'b', 'c']);
    });

    it('should display all values after unshift', async () => {
        // execution
        component.values.unshift({value: 'z'});
        fixture.detectChanges();
        await fixture.whenStable();

        // evaluation
        console.log(JSON.stringify(getAllValues())); // Logs '["z","z","b"]'
        expect(getAllValues()).toEqual(['z', 'a', 'b']);
    });
});

The first two tests pass just fine. However the third test fails. In the third test I attempt to prepend "z" to my inputs, which is successful, however the second input also shows "z", which it should not.

(Note that hundreds of similar questions exist on the web, but in the other cases people were just not having unique name-attributes and they are also just appending, not prepending).

Why is this happening and what can I do about it?

Notes on `trackBy`

So far the answers were just "use trackBy". However the documentation for trackBy states:

By default, the change detector assumes that the object instance identifies the node in the iterable

Since I don't supply an explicit trackBy-Function, that means that angular is supposed to track by identity, which (in the case above) absolutely correctly identifies each object and is inline with what the documentation expects.

The answer by Morphyish basically states that the feature to track by identity is broken and proposes to use an id-Property. At first it seemed to be a solution, but then it turned out to be just an error. Using an id-Property exhibits the exact same behavior as my test above.

The answer by penleychan tracks by index, which causes angular to think, that after I unshifted a value angular thinks that actually I pushed a value and all values in the array just happened to have updated. It kind of works around the issue, but it is in violation to the track-By contract and it defeats the purpose of the track-by-function (to reduce churn in the DOM).

My assumption is probably due to array mutation. What happens if you try using the spread operator, `[{value: 'z'}, ...values]`. — penleychan, Jul 23 '19 at 11:52
@penleychan: I supected that there might be some connection as well, but nope. Copying the array does not change anything. — yankee, Jul 23 '19 at 11:55

Morphyish · Accepted Answer · 2019-08-13T15:55:55.563

Edit

After further investigation, the issue wasn't actually coming from the ngFor. It was the ngModel using the name attribute of the input.

In the loop, the name attribute is generated using the array index. However, when placing a new element at the start of the array, we suddenly have a new element with the same name.

This is probably creating a conflict with multiple ngModel observing the same input internally.

This behavior can be furthered observed when adding multiple inputs at the start of the array. All inputs that were initially created with the same name attribute, will take the value of the new input being created. Regardless if their respective values were changed or not.

To fix this issue you simply need to give each input a unique name. Either by using a unique id, as from my example below

<input [name]="'elem' + item.id" [(ngModel)]="item.value">

Or by using a unique name/id generator (similar to what's Angular Material does).

Original answer

The issue, as stated by penleychan, is the missing trackBy on your ngFor directive.

You can find a working example of what you are looking for here

With the updated code from your example

import { Component } from '@angular/core';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { FormsModule } from '@angular/forms';


@Component({
    template: `
        <form>
            <div *ngFor="let item of values; let index = index; trackBy: trackByFn">
                <input [name]="'elem' + index" [(ngModel)]="item.value">
            </div>
        </form>`
})
class TestComponent {
    values: {id: number, value: string}[] = [{id: 0, value: 'a'}, {id: 1, value: 'b'}];
    trackByFn = (index, item) => item.id;
}

fdescribe('ngFor/Model', () => {
    let component: TestComponent;
    let fixture: ComponentFixture<TestComponent>;
    let element: HTMLDivElement;

    beforeEach(async () => {
        TestBed.configureTestingModule({
            imports: [FormsModule],
            declarations: [TestComponent]
        });

        fixture = TestBed.createComponent(TestComponent);
        component = fixture.componentInstance;
        element = fixture.nativeElement;

        fixture.detectChanges();
        await fixture.whenStable();
    });

    function getAllValues() {
        return Array.from(element.querySelectorAll('input')).map(elem => elem.value);
    }

    it('should display all values', async () => {
        // evaluation
        expect(getAllValues()).toEqual(['a', 'b']);
    });

    it('should display all values after push', async () => {
        // execution
        component.values.push({id: 2, value: 'c'});
        fixture.detectChanges();
        await fixture.whenStable();

        // evaluation
        expect(getAllValues()).toEqual(['a', 'b', 'c']);
    });

    it('should display all values after unshift', async () => {
        // execution
        component.values.unshift({id: 2, value: 'z'});
        fixture.detectChanges();
        await fixture.whenStable();

        // evaluation
        console.log(JSON.stringify(getAllValues())); // Logs '["z","z","b"]'
        expect(getAllValues()).toEqual(['z', 'a', 'b']);
    });
});

Despite your comment, it not a workaround. trackBy was made for the type of use (as well as performances but both are linked).

You can find ngForOf directive code here if you want to take a look for yourself, but here is how it works.

The ngForOf directive is diffing the array to determine modifications made, however without a specific trackBy function passed it is left with making soft comparisons. Which is fine for simple data structure such as strings, or numbers. But when you are using Objects, it can get wacky really fast.

On top of lowering performances, the lack of clear identification for the items inside the array can force the array to re-render the entirety of the elements.

However, if the ngForOf directive is able to clearly determine which item has changed, which item was deleted and which one was added. It can leave every other items untouched, add or remove templates from the DOM as required, and update only the ones that needs to be.

If you add the trackBy function and add an item at the start of the array, the diffing can realize that this is exactly what happened, and prepend a new template a the beginning of the loop while binding the corresponding item to it.

Strange, because the documentation states "By default, the change detector assumes that the object instance identifies the node in the iterable". In this case using the object instance is just as unique as using the id-Property. Thus I assumed, this this would make no difference. However your test proves this assumption wrong. I wonder why this is. Of course this answer is much better then just using an index as id, because using an actual id property actually is an id as requested by the documentation. — yankee, Aug 04 '19 at 16:00
You are right, the instance reference should be enough according to the documentation. And in a sense it is, the new input is detected and added. However it's far from perfect and fucks up the binding with the templates. I'm guessing the issue should be around the `forEachIdentityChange` loop, it's detecting a change in the first item, so it's updating it's binding. Except at that time the first item is actually the second item, so it ends up binding the first item to the second item on top of binding to the actual first item. — Morphyish, Aug 04 '19 at 17:42
I was just about to implement the learnings from your answer and it did not work. After some comparision I found an error in your code: The line reading `trackByFn = item => item.id;` means that the result is ALWAYS undefined. You probably meant `trackByFn = (index, item) => item.id;`, but this again does not work. **This would make this answer invalid**. Could you please double-check? — yankee, Aug 13 '19 at 11:13
Indeed, it is the `name` which causes all the problems. Good catch! I added an ID-Generator like this: `function createObjectIdGenerator() { const objectIds = new WeakMap — yankee, Aug 13 '19 at 14:39

score 6 · Answer 2 · answered Jul 23 '19 at 12:17

6

Figured out the issue. Use trackBy

The issue is if the value changes, the differ reports a change. So if the default function returns object references, it will not match the current item if the object reference has changed.

Explaination on trackBy https://stackoverflow.com/a/45391247/6736888

https://stackblitz.com/edit/angular-testing-gvpdhr

answered Jul 23 '19 at 12:17

penleychan

5,370
1
17
28

Tracking by index does indeed seem to fix the problem. However as far as I understand this will basically mean that if I unshift, then angular thinks I actually pushed a new item to the array and all the `value`-properties seem to have changed. It feels more like a workaround, rather then a solution. – yankee Jul 23 '19 at 17:53
This is the solution, but you need to provide a unique identifier for each object in the array you are iterating over through the trackBy method that will not change, so the ngFor can correctly identify each object. – Morphyish Jul 31 '19 at 16:39
@yankee angular is a pretty smart framework. The same thing I was thinking when I was using `*ngFor` with `formArray.controls` thus working with arrays of inputs/groups of inputs in ReactiveForms. It turned out that when the array changes the Angular doesn't re-create elements. It tracks the references and only updates those what changed. It doesn't matter what kind of reference. (I was using default Object reference since I was iterating over controls in FormArray). I know it because I had components and I tracked their lifecycle – Sergey Aug 02 '19 at 19:05

ngFor + ngModel: How can I unshift values to the array I am iterating?

Notes on trackBy

2 Answers2

Notes on `trackBy`