fix(table): update implicit when using trackby #7893

andrewseguin · 2017-10-18T22:31:43Z

The table should make sure that if it wants to re-use a row, then it's implicit data should be accurate. It's possible that a row's data is entirely changed but the row was re-used due to a loose trackBy.

Fixes #7877

jelbourn · 2017-10-21T00:33:49Z

src/cdk/table/table.ts

@@ -68,6 +68,14 @@ export const CDK_TABLE_TEMPLATE = `
  <ng-container headerRowPlaceholder></ng-container>
  <ng-container rowPlaceholder></ng-container>`;

+/** Class used to conveniently type the embedded view ref for rows with a context. */
+abstract class RowViewRef<T> extends EmbeddedViewRef<CdkCellOutletRowContext<T>> { }


Should this be @docs-private?

jelbourn · 2017-10-21T00:33:55Z

src/cdk/table/table.ts

+abstract class RowViewRef<T> extends EmbeddedViewRef<CdkCellOutletRowContext<T>> { }
+
+/** Tuple used to pair together an iterable differ change record with the view it affects. */
+class RowRecordViewTuple<T> {


@docs-private?

jelbourn · 2017-10-21T00:34:18Z

src/cdk/table/table.ts

+
+/** Tuple used to pair together an iterable differ change record with the view it affects. */
+class RowRecordViewTuple<T> {
+  constructor(public record: IterableChangeRecord<T>, public view: RowViewRef<T>) {}


Should record and view be public, documented properties?

Also neither seem to be the most descriptive names

jelbourn · 2017-10-21T00:35:42Z

src/cdk/table/table.ts

  private _renderRowChanges() {
    const changes = this._dataDiffer.diff(this._data);
    if (!changes) { return; }

+    const insertOrMovedRowTuples: RowRecordViewTuple<T>[] = [];


Add comment that explains why you keep track of the inserted or moved tuples?

jelbourn · 2017-10-21T00:37:10Z

src/cdk/table/table.ts

+
+    // Update rows that did not get added/removed/moved but may have had their identity changed,
+    // e.g. if trackBy matched data on the property 'id' but the property 'name' was changed.
+    changes.forEachIdentityChange((record: any) => {


jelbourn · 2017-10-21T00:37:58Z

src/cdk/table/table.ts

  }

  /**
-   * Updates the context for each row to reflect any data changes that may have caused
+   * Updates the meta context for each row to reflect any data changes that may have caused


"Meta context"?

jelbourn · 2017-10-21T00:38:55Z

src/cdk/table/table.ts

+  }
+
+  /** Updates the row view ref with the implicit data assigned to the row. */
+  private _updateRowImplicitContext(view: RowViewRef<T>, record: IterableChangeRecord<any>) {


I think the code would be easier to read without this method (just having the assignment inline). Any reason to keep it a method?

jelbourn · 2017-10-21T00:40:45Z

src/cdk/table/table.ts

          }
        });

-    this._updateRowContext();
+    // Update the row's implicit context for each inserted or moved row.


"Update the $implicit property for the binding context for inserted and moved rows"

andrewseguin · 2017-10-23T17:34:56Z

tl;dr: Took out logic about insertions and moves. Just updating for identity changes since it addresses the issue.

Reducing the scope of this PR - was looking at Angular's ngFor (ng_for_of.ts) to see how they addressed this issue. They updated row $implicit on insert/move and also for any identity changes. The issue was fixed with the identity change and I'm not convinced we need the former (updating on insert/move, which should be addressed already on insert).

If another issue comes in about bad $implicit on moves then we'll know where to look, but I'm not 100% convinced it's needed.

jelbourn

LGTM, one syntax comment

Add the "merge-ready" label when done

jelbourn · 2017-10-23T18:58:15Z

src/cdk/table/table.spec.ts

+      // Change each item reference to show that the trackby is checking the index.
+      // Otherwise this would cause them all to be removed/added.
+      trackByComponent.dataSource.data = trackByComponent.dataSource.data
+          .map(item => { return {a: item.a, b: item.b, c: item.c}; });


.map(item => ({a: item.a, b: item.b, c: item.c}));

angular-automatic-lock-bot · 2019-09-07T20:02:26Z

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.}

andrewseguin added mat-table labels Oct 18, 2017

andrewseguin requested a review from jelbourn October 18, 2017 22:31

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 18, 2017

andrewseguin added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Oct 20, 2017

jelbourn reviewed Oct 21, 2017

View reviewed changes

andrewseguin force-pushed the table-change-context branch from 36257d8 to 3d09957 Compare October 23, 2017 17:29

fix(table): update implicit when using trackby

c2e9469

andrewseguin force-pushed the table-change-context branch from 3d09957 to c2e9469 Compare October 23, 2017 17:30

imports

9e10265

jelbourn approved these changes Oct 23, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Oct 23, 2017

syntax change

f213b00

andrewseguin added the action: merge The PR is ready for merge by the caretaker label Oct 24, 2017

mmalerba merged commit f806286 into angular:master Oct 27, 2017

andrewseguin deleted the table-change-context branch November 28, 2017 20:38

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(table): update implicit when using trackby #7893

fix(table): update implicit when using trackby #7893

andrewseguin commented Oct 18, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

jelbourn Oct 21, 2017

andrewseguin commented Oct 23, 2017 •

edited

Loading

jelbourn left a comment

jelbourn Oct 23, 2017

angular-automatic-lock-bot bot commented Sep 7, 2019

fix(table): update implicit when using trackby #7893

fix(table): update implicit when using trackby #7893

Conversation

andrewseguin commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Oct 23, 2017 • edited Loading

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 7, 2019

andrewseguin commented Oct 23, 2017 •

edited

Loading