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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MatTree does not update items when using trackBy #18639

Open
matthiaswelz opened this issue Feb 27, 2020 · 12 comments · May be fixed by #26508
Open

MatTree does not update items when using trackBy #18639

matthiaswelz opened this issue Feb 27, 2020 · 12 comments · May be fixed by #26508
Labels
area: cdk/tree area: material/tree P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@matthiaswelz
Copy link

matthiaswelz commented Feb 27, 2020

When using trackBy, CdkTree / MatTree does not update items which share the same identity (as returned by trackBy), but are actually different obejcts (with property changes).
This behavior differs from the behavior shown by ngFor in combination with trackBy.

Reproduction

StackBlitz: https://stackblitz.com/edit/angular-cfph4s

Steps to reproduce:

  1. Click the button

Expected Behavior

The tree gets updated as the list created by ngFor does.

Actual Behavior

The tree does not change, only the list generated by ngFor.

Environment

  • Angular: 9.0.1
  • CDK/Material: 9.1.0
  • Browser(s): Chrome
  • Operating System (e.g. Windows, macOS, Ubuntu): macOS
@matthiaswelz matthiaswelz changed the title CdkTree does not update items when using trackBy MatTree does not update items when using trackBy Feb 27, 2020
@mmalerba mmalerba added the needs triage This issue needs to be triaged by the team label May 20, 2020
@devversion
Copy link
Member

Thanks for this issue. This seems like a valid bug to me. The NgForOf directive also respects the identity, correct. See:

https://cs.opensource.google/angular/angular/+/master:packages/common/src/directives/ng_for_of.ts;l=260-264?q=NgFor&ss=angular

We should probably add the same to the tree here, but not sure if it would be considered breaking.

@devversion devversion added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent area: cdk/tree and removed needs triage This issue needs to be triaged by the team labels Aug 7, 2020
@rklfss
Copy link

rklfss commented Jan 28, 2021

Any news on this one?

I ran into the same issue and quick and dirty fixed it by using a custom tree component.

I think, we have to get the instance of MatTreeNode/CdkTreeNode and set the data property, but I don't know how to get it. So I just ended up overriding the outlet context.

@Component({
  selector: 'app-fixed-tree',
  template: '<ng-container matTreeNodeOutlet></ng-container>',
  host: {
    'class': 'mat-tree cdk-tree app-fixed-tree',
    'role': 'tree',
  },
  // you might want to include the entire styles from tree.scss, but it is not needed in my case
  styles: ['.app-fixed-tree {display:block;}'],
  encapsulation: ViewEncapsulation.None,
  changeDetection: ChangeDetectionStrategy.Default,
  providers: [{
    provide: CdkTree,
    useExisting: FixedTreeComponent,
  }],
})
export class FixedTreeComponent<T, K = T> extends MatTree<T, K> {
  renderNodeChanges(data: T[] | ReadonlyArray<T>, dataDiffer?: IterableDiffer<T>, viewContainer: ViewContainerRef = this._nodeOutlet.viewContainer, parentData?: T): void {
    super.renderNodeChanges(data, dataDiffer, viewContainer, parentData);

    // apply new data objects
    if (this.trackBy != null) {
      data.forEach((d, i) => {
        const view: any = viewContainer.get(i);
        if (view && view.context) {
          view.context.$implicit = d;
        }
      });
    }
  }
}

@jquartz
Copy link

jquartz commented Jan 28, 2021

Any news on this one?

I ran into the same issue and quick and dirty fixed it by using a custom tree component.

I think, we have to get the instance of MatTreeNode/CdkTreeNode and set the data property, but I don't know how to get it. So I just ended up overriding the outlet context.

@Component({
  selector: 'app-fixed-tree',
  template: '<ng-container matTreeNodeOutlet></ng-container>',
  host: {
    'class': 'mat-tree cdk-tree app-fixed-tree',
    'role': 'tree',
  },
  // you might want to include the entire styles from tree.scss, but it is not needed in my case
  styles: ['.app-fixed-tree {display:block;}'],
  encapsulation: ViewEncapsulation.None,
  changeDetection: ChangeDetectionStrategy.Default,
  providers: [{
    provide: CdkTree,
    useExisting: FixedTreeComponent,
  }],
})
export class FixedTreeComponent<T, K = T> extends MatTree<T, K> {
  renderNodeChanges(data: T[] | ReadonlyArray<T>, dataDiffer?: IterableDiffer<T>, viewContainer: ViewContainerRef = this._nodeOutlet.viewContainer, parentData?: T): void {
    super.renderNodeChanges(data, dataDiffer, viewContainer, parentData);

    // apply new data objects
    if (this.trackBy != null) {
      data.forEach((d, i) => {
        const view: any = viewContainer.get(i);
        if (view && view.context) {
          view.context.$implicit = d;
        }
      });
    }
  }
}

This still leads to the component not updating, plus type of FixedTreeComponent should take one argument
FixedTreeComponent<T>

@rklfss
Copy link

rklfss commented Jan 29, 2021

This still leads to the component not updating

Ok, really? Maybe it's because I update the expansion model right after updating the source which leads to a new change detection. Then it's just a works-for-me-fix, not worth to mention.

plus type of FixedTreeComponent should take one argument FixedTreeComponent<T>

That's not true. K is the type of the trackBy function result and we have to pass it to the MatTree.

@jpduckwo
Copy link

I think I might be affected by this too. Once I add a trackBy function to the tree when refreshing the data, the toggling breaks

@cesco69
Copy link

cesco69 commented Mar 1, 2022

same issue for me...

@dirkluijk
Copy link
Contributor

Same issue, seems to be an issue with CDK Tree. Can't believe this hasn't been fixed over 2 years.

@dirkluijk
Copy link
Contributor

dirkluijk commented Jan 26, 2023

I tried to implement a solution, but so far, it seems that the current implementation has some clear flaws and shortcomings that prevent an easy fix.

This is my analysis so far:

  1. CdkTree does not apply any updates on "identity changes" in the IterableDiffer. I tried fixing this in the CdkTree.renderNodeChanges():
changes.forEachIdentityChange((record) => {
  if (record.currentIndex !== null) {
    console.log('processing', data[record.currentIndex]);
    const viewRef = <EmbeddedViewRef<CdkTreeNodeOutletContext<T>>>viewContainer.get(record.currentIndex);
    viewRef.context.$implicit = record.item;
  }
});

That fixes one issue.

  1. The BaseTreeControl (which NestedTreeControl extends) has a trackBy property that is not set. Manually setting it in its constructor fixes that issue: treeControl = new NestedTreeControl<FoodNode, string>(node => node.children, { trackBy: (node) => node.id });. In my opinion, this should be handled by CdkTree automatically or documented more clearly.
  2. At this point, it works as expected, but only on the first level. Child nodes are still not updated. However, it seems that the current implementation doesn't allow existing child nodes to get data updates. CdkTree relies on some weird static variable CdkTreeNode.mostRecentTreeNode that keeps track of the latest rendered node in order to set the data on it, but that doesn't work with data updates.

@dirkluijk
Copy link
Contributor

dirkluijk commented Jan 26, 2023

I shared my fixes in this pull request: #26508.

Those fixes are only part of the solution, I still rely on a workaround (see PR). Any contributions, ideas or feedback is welcome. I am also considering a custom implementation.

@JonasDev17
Copy link

The issue seems to be still present in 2024

@JonasDev17
Copy link

@devversion Do you have any plans on fixing this in an upcoming version?

This prevents users from optimizing their trees for performance and a side effect of re-rendering the entire tree is that you cannot use css transitions for e.g. the expansion icon as all icons will be re-rendered and all icons will be animated / rotated again.

@devversion
Copy link
Member

Sorry @JonasDev17, I'm not actively working on Angular Components, but maybe a PR can be created that re-uses some of the logic from NgForOf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/tree area: material/tree 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.

10 participants