feat(cdk/collections): extract view repeater strategies #19964

MichaelJamesParsons · 2020-07-13T23:10:48Z

A ViewRepeater implements a strategy for rendering a list of elements inside a common ViewContainerRef. It encapsulates all the business logic pertaining to inserting, moving, and deleting embedded views. This PR extracts existing strategies from virtualForOf and the CDK table, so we can reuse them in upcoming virtual scroll work.

Summary

Extracts view repeater strategies from virtualForOf and CDK table.
Refactors virtualForOf and CDK table to use the strategies.
With the exception of extracting the view repeater strategies, no behavior has changed.

For now, this API will be internal until we've tested it with enough components to know it's stable.

src/cdk/collections/view-repeater.ts

src/cdk/scrolling/virtual-for-of.ts

src/cdk/collections/recycle-view-repeater-strategy.ts

goldens/ts-circular-deps.json

src/cdk/collections/recycle-view-repeater-strategy.ts

src/cdk/table/table-module.ts

tools/public_api_guard/cdk/collections.d.ts

src/cdk/table/table.ts

jelbourn · 2020-07-14T20:49:40Z

src/cdk/table/table.ts

-  providers: [{provide: CDK_TABLE, useExisting: CdkTable}]
+  providers: [
+    {provide: CDK_TABLE, useExisting: CdkTable},
+    {provide: _VIEW_REPEATER_STRATEGY, useClass: DisposeViewRepeaterStrategy},


Providing this directly might still end up with the problem of always including the code for DisposeViewRepeaterStrategy even when it's not being used. Is this a temporary thing until we have a directive API for switching between rending strategies?

Yes, this is temporary. The virtual scroll table PR will introduce a directive-based API to replace it.

Cool- can you add a TODO here to make that explicit?

googlebot · 2020-07-23T17:25:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-07-23T17:25:57Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-07-23T19:09:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

MichaelJamesParsons · 2020-07-24T01:24:31Z

The failing tests are related to the MDC MatSlider. The changes made in this PR shouldn't have impacted them.

mmalerba

LGTM

mmalerba · 2020-07-24T20:59:12Z

src/cdk/collections/recycle-view-repeater-strategy.ts

+ */
+export class _RecycleViewRepeaterStrategy<T, R, C extends _ViewRepeaterItemContext<T>>
+    implements _ViewRepeater<T, R, C> {
+  differ: IterableDiffer<C>;


Is this used anywhere? I don't see it

The differ was abstracted out of the repeater strategy. I'll remove it.

jelbourn

LGTM aside from minor remaining comments; I'll leave it to @mmalerba for final approval

jelbourn · 2020-07-24T23:09:34Z

src/cdk/scrolling/virtual-for-of.ts

+  get cdkVirtualForTemplateCacheSize() {
+    return this._viewRepeater.viewCacheSize;
+  }
+  set cdkVirtualForTemplateCacheSize(size: number | string) {


This should just be number, and you can add this to the bottom of this class to tell Angular to accept other data types:

static ngAcceptInputType_cdkVirtualForTemplateCacheSize: NumberInput;

MichaelJamesParsons · 2020-07-25T17:45:25Z

@mmalerba The latest revisions are complete.

jelbourn · 2020-07-27T20:53:17Z

@MichaelJamesParsons looks like the PR just needs to be rebased

mmalerba

LGTM

mmalerba · 2020-07-27T21:18:08Z

Looks like there's a lint issue that needs to be fixed, otherwise ready to go

…orOf and CDK table.

MichaelJamesParsons · 2020-07-27T22:16:35Z

@jelbourn @mmalerba All fixed

jelbourn · 2020-07-28T05:02:27Z

@googlebot ping

In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy. These changes make the new constructor parameters optional and add fallbacks to the old behavior. Fixes angular#20422.

angular-automatic-lock-bot · 2020-08-30T00:13:24Z

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

MichaelJamesParsons requested review from andrewseguin, crisbeto and jelbourn as code owners July 13, 2020 23:10

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 13, 2020

MichaelJamesParsons requested a review from mmalerba July 13, 2020 23:11

MichaelJamesParsons force-pushed the view-repeaters branch from 4da1980 to 62829de Compare July 13, 2020 23:15

MichaelJamesParsons added area: cdk/collections G This is is related to a Google internal issue labels Jul 13, 2020

jelbourn added the target: minor This PR is targeted for the next minor release label Jul 14, 2020

MichaelJamesParsons force-pushed the view-repeaters branch 3 times, most recently from 0706a2a to f8f33eb Compare July 14, 2020 01:06

mmalerba reviewed Jul 14, 2020

View reviewed changes

src/cdk/collections/view-repeater.ts Show resolved Hide resolved

mmalerba reviewed Jul 14, 2020

View reviewed changes

src/cdk/collections/view-repeater.ts Outdated Show resolved Hide resolved

src/cdk/collections/view-repeater.ts Outdated Show resolved Hide resolved

src/cdk/collections/view-repeater.ts Show resolved Hide resolved

src/cdk/scrolling/virtual-for-of.ts Outdated Show resolved Hide resolved

MichaelJamesParsons force-pushed the view-repeaters branch 3 times, most recently from 5ea2236 to 7e50f47 Compare July 14, 2020 04:25

MichaelJamesParsons requested a review from a team as a code owner July 14, 2020 04:25

jelbourn reviewed Jul 14, 2020

View reviewed changes

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 23, 2020

MichaelJamesParsons force-pushed the view-repeaters branch 2 times, most recently from ae357b4 to 8b99d2c Compare July 23, 2020 19:09

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 23, 2020

MichaelJamesParsons force-pushed the view-repeaters branch 2 times, most recently from 29e13bf to a02a01e Compare July 23, 2020 22:38

MichaelJamesParsons force-pushed the view-repeaters branch from a02a01e to 7235609 Compare July 23, 2020 22:56

mmalerba approved these changes Jul 24, 2020

View reviewed changes

jelbourn approved these changes Jul 24, 2020

View reviewed changes

jelbourn added the lgtm label Jul 24, 2020

MichaelJamesParsons force-pushed the view-repeaters branch from 7235609 to 3bdeaff Compare July 25, 2020 17:43

MichaelJamesParsons force-pushed the view-repeaters branch from 3bdeaff to 261f1ba Compare July 27, 2020 21:12

mmalerba approved these changes Jul 27, 2020

View reviewed changes

mmalerba added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround labels Jul 27, 2020

MichaelJamesParsons force-pushed the view-repeaters branch from 261f1ba to 3410779 Compare July 27, 2020 21:18

feat(cdk/collections): extract view repeater strategies from virtualF…

3c5137f

…orOf and CDK table.

MichaelJamesParsons force-pushed the view-repeaters branch from 3410779 to 3c5137f Compare July 27, 2020 21:51

jelbourn merged commit 3b3b55f into angular:master Jul 29, 2020

MichaelJamesParsons deleted the view-repeaters branch July 30, 2020 19:06

crisbeto mentioned this pull request Aug 26, 2020

fix(cdk/table): resolve breaking constructor changes #20425

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 30, 2020

feat(cdk/collections): extract view repeater strategies #19964

feat(cdk/collections): extract view repeater strategies #19964

Uh oh!

Conversation

MichaelJamesParsons commented Jul 13, 2020

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 23, 2020

Uh oh!

googlebot commented Jul 23, 2020

Uh oh!

googlebot commented Jul 23, 2020

Uh oh!

MichaelJamesParsons commented Jul 24, 2020

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelJamesParsons commented Jul 25, 2020

Uh oh!

jelbourn commented Jul 27, 2020

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Jul 27, 2020

Uh oh!

MichaelJamesParsons commented Jul 27, 2020

Uh oh!

jelbourn commented Jul 28, 2020

Uh oh!

angular-automatic-lock-bot bot commented Aug 30, 2020

Uh oh!

Uh oh!