Skip to content

feat(data-table): use iterate differ to optimize rendering #4823

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

Merged
merged 5 commits into from
Jun 5, 2017

Conversation

andrewseguin
Copy link
Contributor

No description provided.

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

// Swap first and second data in data array
let copiedData = component.dataSource.data.slice();
let temp = copiedData[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let -> const in both of these

@@ -92,9 +98,13 @@ export class CdkTable implements CollectionViewer {
/** Set of templates that used as the data row containers. */
@ContentChildren(CdkRowDef) _rowDefinitions: QueryList<CdkRowDef>;

constructor(private _changeDetectorRef: ChangeDetectorRef) {
constructor(private _differs: IterableDiffers,
private _changeDetectorRef: ChangeDetectorRef) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should both of these be readonly as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we've set that precedent, but I can talk with Jeremy about adding it to here and other components as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they should be readonly; we started the library before readonly was a thing and never got in the habit of adding it later

@@ -76,6 +79,9 @@ export class CdkTable implements CollectionViewer {
*/
private _columnDefinitionsByName = new Map<string, CdkColumnDef>();

/** Differ used to find the changes in the data provided by the data source. */
private _dataDiffer: IterableDiffer<any> = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we should consider doing before this is released is making it CdkTable<T> and using the generic throughout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a generic to the CdkTable definition

console.warn('The data table is still in active development ' +
'and should be considered unstable.');

// TODO(andrewseguin): Add trackby function input.
this._dataDiffer = this._differs.find([]).create();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow how this works; why is this find necessary? Can you not just create an IterableDiffer directly? Why is the DI system involved?
(probably just needs explanation in comments)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comment mentioning that we need to find an iterable differ used for array-based iterables. It's the only public API that lets us grab it, no luck instantiating one myself.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review action: merge The PR is ready for merge by the caretaker labels Jun 1, 2017
@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2017

Just noticed still needs readonly per Erin's comment

@andrewseguin
Copy link
Contributor Author

Thanks for catching that, got the readonly in, thought I had changed that already.

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Jun 5, 2017
@andrewseguin andrewseguin merged commit 181820e into angular:master Jun 5, 2017
@andrewseguin andrewseguin deleted the table-differs branch November 28, 2017 20:35
@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 Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants