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.

Sign up for GitHub

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

Jump to bottom

fix(cdk/scrolling): virtual list not updating when source array is mutated #14639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(cdk/scrolling): virtual list not updating when source array is mutated #14639

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

When an array is passed to cdkVirtualFor, it falls back to creating an ArrayDataSource which won't emit if the array has changed. Since the intention of the cdkVirtualFor is to be (more or less) a drop-in alternative for ngFor, it is expected that it'll update if the data has changed. These changes add a new type of data source that will allow us to detect changes on the data and to update the view.

Note: I went with adding a new type of data source, rather than using the iterable differ directly, because it would've meant having to maintain two different code paths inside the directive. Furthermore, I can see this being useful in cdk/tree and cdk/table as well.

Fixes #14635.
Fixes #14501.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Dec 26, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 26, 2018
@crisbeto crisbeto force-pushed the 14635/virtual-scroll-array-changes branch from 9fd7f78 to 772d621 Compare December 26, 2018 10:09
@crisbeto crisbeto requested a review from devversion as a code owner December 26, 2018 10:09
mmalerba
mmalerba reviewed Jan 2, 2019
View reviewed changes
src/cdk/scrolling/virtual-for-of.ts
* Current differ data source. Needs to be kept in a separate
* property so we can run change detection on it.
*/
private _differDataSource: DifferDataSource<T> | null = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this... it seems a little tricky to manage. Would it make sense to change all DataSources to have these new methods? They could just be no-op by default

@jelbourn

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm not a fan of it either, but it was cleaner than doing it inside the virtual repeater. An alternative can be to roll this functionality into the ArrayDataSource.

@crisbeto crisbeto force-pushed the 14635/virtual-scroll-array-changes branch from 772d621 to eb25578 Compare January 30, 2019 18:58
@crisbeto
Copy link
Member Author

@jelbourn @mmalerba how should we proceed with this one?

@mmalerba
Copy link
Contributor

Why don't we just add the methods to DataSource with a default implementation that does nothing? That seems like the cleanest solution to me

@jelbourn WDYT?

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@crisbeto crisbeto force-pushed the 14635/virtual-scroll-array-changes branch from eb25578 to e206d12 Compare May 24, 2019 17:34
@andrewseguin andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label May 30, 2019
@mmalerba mmalerba removed their assignment Aug 12, 2019
@MayuriRathod
Copy link

Is the issue fixed?
when will this be available for us?

@crisbeto crisbeto force-pushed the 14635/virtual-scroll-array-changes branch from e206d12 to 2df2b2c Compare February 9, 2020 16:45
@crisbeto crisbeto force-pushed the 14635/virtual-scroll-array-changes branch from 2df2b2c to cc127ab Compare March 6, 2020 21:28
@Raigedas
Copy link

can we approve the fix now? even thou it is not "perfect" it seems to me it does not impact other areas. later it could be improved but at the moment we need the functionality which is even in basic ion-list (in ionic).
thank you.

andrewseguin
andrewseguin reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

I think having this sort of DataSource seems neat - users get confused about how they make a data source emit when its based on an array but they keep mutating the contents of their array.

Could the _iterable property be changed to public so that users can set a different array, which would trigger a doCheck?

@andrewseguin andrewseguin self-requested a review June 15, 2020 19:31
andrewseguin
andrewseguin reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

.

@crisbeto crisbeto changed the title fix(scrolling): virtual list not updating when source array is mutated fix(cdk/scrolling): virtual list not updating when source array is mutated Feb 17, 2021
@crisbeto
fix(cdk/scrolling): virtual list not updating when source array is mu…
42ed989 
…tated

When an array is passed to `cdkVirtualFor`, it falls back to creating an `ArrayDataSource`
which won't emit if the array has changed. Since the intention of the `cdkVirtualFor` is to
be (more or less) a drop-in alternative for `ngFor`, it is expected that it'll update if the data
has changed. These changes add a new type of data source that will allow us to detect
changes on the data and to update the view.

Fixes angular#14635.
Fixes angular#14501.
@crisbeto crisbeto force-pushed the 14635/virtual-scroll-array-changes branch from cc127ab to 42ed989 Compare February 17, 2021 20:30
@devversion devversion removed their request for review August 18, 2021 12:55
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@andrewseguin andrewseguin removed the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Mar 28, 2022
@josephperrott josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@josephperrott josephperrott requested review from a team as code owners December 18, 2024 17:40
@josephperrott josephperrott requested review from amysorto and removed request for a team December 18, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmalerba mmalerba

@andrewseguin andrewseguin

@jelbourn jelbourn

@amysorto amysorto

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
10 participants
@crisbeto @mmalerba @MayuriRathod @Raigedas @andrewseguin @jelbourn @devversion @googlebot @amysorto @josephperrott