Skip to content
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.

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

Already on GitHub? Sign in to your account

Improve documentation of behavior for ngFor's trackBy #40461

Closed
Doc999tor opened this issue Jan 17, 2021 · 13 comments
Closed

Improve documentation of behavior for ngFor's trackBy #40461

Doc999tor opened this issue Jan 17, 2021 · 13 comments
Assignees
Labels
area: core Issues related to the framework runtime core: differs P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone

Comments

@Doc999tor
Copy link

Doc999tor commented Jan 17, 2021

馃悶 bug report

Affected Package

The issue is caused by package @angular/core

Description

Array of objects (interface User { id : number, name: string }) is rendered by *ngFor with trackBy

  identify(index: number, item: User) {
    return item.id;
  }

The items in the array are added and deleted by the client implementation
Instead of relying on id of each element, the array items their indices changed are reordered, are being rerendered
Even though I am passing item.id in the trackBy function, Angular is re-rendering the array items when the order of the items in the array changes
So I suppose trackBy will identify only changed items and rerender them only but every moved item is rerendered as well - unexpected behavior
Demo

馃敩 Minimal Reproduction

import { Component } from "@angular/core";

interface User {
  id: number;
  name: string;
}

@Component({
  selector: "my-app",
  templateUrl: "./app.component.html",
  styleUrls: ["./app.component.css"]
})
export class AppComponent {
  users: User[];

  constructor() {
    this.change1();
  }

  change1(): void {
    this.users = [
      { id: 1, name: "one" },
      { id: 2, name: "two" },
      { id: 3, name: "three" },
      { id: 4, name: "four" },
      { id: 5, name: "five" },
    ];
  }
  change2(): void {
    this.users = [
      { id: 3, name: "three" },
      { id: 5, name: "five" },
      { id: 1, name: "one" },
      { id: 2, name: "two" },
      { id: 6, name: "six" },
    ];
  }

  identify(index: number, item: User) {
    return item.id;
  }

馃實 Your Environment

Angular Version:
Angular version - v10.1.5

@Doc999tor Doc999tor changed the title ngFor trackBy incorrect behavior on array index changing ngFor trackBy incorrect behavior when array index is changing Jan 17, 2021
@petebacondarwin
Copy link
Member

It is interesting to note that only items that are moved "earlier" in the list are re-rendered. When "one" and "two" are moved later in the array, by items being added/moved ahead of them, they do not re-render.
I expect that this is by design to avoid having to search the whole array on each change detection, but perhaps @mhevery can confirm.

@givo
Copy link

givo commented Jan 18, 2021

Hi,

I am facing the same problem.
@petebacondarwin If what you are saying is true, I think there's is a bug in the design of trackBy.

If we choose to use the ID of an object for tracking changes, the array stays in the same size and only the order of the items changes but not the IDs of the objects, we do not want the items to re-render.

This behavior is an exactly what I expect from the definition of trackBy as described in Angular Docs:
"When supplied, Angular tracks changes by the return value of the function"

Best regards

@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime core: differs labels Jan 19, 2021
@ngbot ngbot bot modified the milestone: needsTriage Jan 19, 2021
@mhevery
Copy link
Contributor

mhevery commented Jan 19, 2021

I have played with the demo and everything is working as intended as far as I can tell which leads me to believe that I don't understand the issue.

First the reproduction needs instructions as to what I should click on and what the expectations vs actual behavior is. Because, when I click on changes it looks fine to me, so unless I understand where your expectations differ from what is happening it is hard for me to understand the issue.

I have few hunches as to what the issue may be.

First let's talk about rendering. Perhaps documentation needs to be updated, but trackBy has no effect on rendering. The rendering is controlled by ChangeDetectionStrategy not by trackBy.

The purpose of trackBy is to better support animation. Imagine you have [{name: 'A'}, {name:'B'}] and you change it to [{name: 'B'}, {name:'A'}] What exactly should happened? There are multiple interpretations.

  • Default behavior: Change A => B in first row AND change B => A on second row. (No change in DOM structure, just change the text, hence no animation)
  • trackBy behavior: Recognize that order of the row 1 and 2 changed as so row 1 should be remove and re-inserted after row 2. This behavior can be used to animate the movement of row 1 to after row 2.

There is no way to know which behavior the developer intended, hence we provide trackBy to tell Angular which should happen.

A way to think about trackBy is that it ensures that the identity of object in the array and the corresponding DOM stay associated. So moves/removes in array and in DOM are in sync.

When looking at your reproduction the array is changed so as to re-order the rows. ngForOf than does just that, rows get reordered.

So is this an issue with better documenting of what trackBy does?

@givo
Copy link

givo commented Jan 19, 2021

Ok, this makes things a little bit more clear, but still I don't understand how trackBy affect the animation.

I would like to provide an example:
I have a component which renders a list of items from an async channel on the screen. On each new batch of items the component should render the new state of the items on the screen and trigger a fade-in animation for new items. This is how I use ngFor and trackBy:

@Component({
    selector: "app-items",
    templateUrl: "./items.component.html",
    styleUrls: ["./items.component.scss"],
})
export class ItemsComponent implements OnInit {
    items$: Observable<Array<Item>>;

    constructor(
        private readonly comService: CommunicationService
    ) { }

    ngOnInit() {
        this.items$ = this.comService.subscribe();
    }

   trackById(index, item: Item) {
      return item.id;
   }
 <ng-container *ngFor="let item of items$ | async; trackBy: trackById">`
		<app-item [item]="item" class="fade-id-animation">
                </app-item>
</ng-container>`

The problem is that the fade in animation is triggered whenever the order of the array changes and not when a new item with a new ID is inserted to the list. Reading the documentation of trackBy makes you think that this is how one should implement this kind of behavior. What am I missing?

@mhevery
Copy link
Contributor

mhevery commented Jan 20, 2021

Angular needs to associated the DOM nodes with an item in the ngForOf array. Normally that association is done through position. As in the first item in array gets first item in the DOM. So when you change [{name: 'A'}, {name:'B'}] and you change it to [{name: 'B'}, {name:'A'}] Angular will simply update the DOM rather than trigger the animation. In your case you need trackBy so that the Angular understands that the items have moved around. As a result angular will move the DOM nodes around as well. The issue is that DOM has no move operation. The only thing DOM has is remove and re-insert operation which is what happens in your case. So if you have animation trigger on DOM insert it will behave just as you showed it. If you want to animate moves you need to give Angular more hints: https://angular.io/guide/animations

This may be useful: https://ultimatecourses.com/blog/angular-animations-how-to-animate-lists

See: https://filipows.github.io/angular-animations/ and https://stackblitz.com/edit/angular-animations-lib-demo

@givo
Copy link

givo commented Jan 20, 2021

Still doesn't make sense to me.

I created a simple test where I use a static size array with a few items (each one has an unique ID) and trackById() function.
I added an interval function which just reorder the items in the array, each time it happens, the animation is triggered.

Doesn't trackBy suppose to prevent any changes to the DOM whenever I pass an ID?

@petebacondarwin
Copy link
Member

Perhaps this example helps to clarify: https://stackblitz.com/edit/angular-ivy-d6avrf?file=src%2Fapp%2Fapp.component.html

In this you can see that the animations are indicating that items have moved in the track by id and object identity scenarios.

Compare this to the track by index scenario, ngFor is unable to tell that the objects have reordered. Instead it assumes that they have not changed order (since the index never changes for each stamped out template) and just updates the internals of the template to match the new data.

@mhevery
Copy link
Contributor

mhevery commented Jan 20, 2021

Perhaps you are trying to do something which requires Angular animations (not just browser?) https://stackblitz.com/edit/angular-ivy-dwln8h?file=src/app/app.component.html

@givo
Copy link

givo commented Jan 20, 2021

@petebacondarwin I still don't get it. Why in your example ngFor with indentify and a plain ngFor acts the same? trackBy didn't do anything. If I tell Angular to track the items by the ID property it's suppose to know there wasn't any change because the list always contains the same IDs. Therefore it's suppose to do nothing, why is the animation being triggered?

@petebacondarwin
Copy link
Member

The reason why the "trackBy id" and "plain ngFor" are the same in my example is because I am reusing the same actual objects - just reordering them. The point of trackBy is to allow new objects to be provided as elements of the array, but for these new elements to be associated with their equivalent elements from before.

const obj1 = {id: 1};
const obj2 = {id: 1};
obj1 !== obj2
obj1.id === obj2.id

without the trackBy the ngFor directive has no way of knowing that if you replaced obj1 with obj2 it is supposed to assume that there has been no removal/addition/move in terms of the elements of the array.

@aikithoughts
Copy link
Contributor

I wonder if we can provide better/clearer documentation for trackby. I'm taking this issue as a documentation one.

@aikithoughts aikithoughts self-assigned this Jan 21, 2021
@jelbourn jelbourn added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Jan 25, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jan 25, 2021
@jelbourn jelbourn changed the title ngFor trackBy incorrect behavior when array index is changing Improve documentation of behavior for ngFor's trackBy Jan 25, 2021
@aikithoughts aikithoughts removed their assignment May 21, 2021
@IgorMinar IgorMinar self-assigned this May 25, 2021
IgorMinar added a commit to IgorMinar/angular that referenced this issue May 25, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461
IgorMinar added a commit to IgorMinar/angular that referenced this issue May 25, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461
IgorMinar added a commit to IgorMinar/angular that referenced this issue May 25, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461
@IgorMinar
Copy link
Contributor

I've cleaned up the docs in #42329, hopefully these improvements clarifies the purpose of track by and clear up the confusion.

zarend pushed a commit that referenced this issue May 26, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes #40461

PR Close #42329
@zarend zarend closed this as completed in 14abc68 May 26, 2021
umairhm pushed a commit to umairhm/angular that referenced this issue May 28, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461

PR Close angular#42329
iRealNirmal pushed a commit to iRealNirmal/angular that referenced this issue Jun 4, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461

PR Close angular#42329
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime core: differs P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants