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

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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cdk/collections/array-data-source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {Observable, isObservable, of as observableOf} from 'rxjs';
import {DataSource} from './data-source';


/** DataSource wrapper for a native array. */
/** DataSource wrapper for a static native array. */
export class ArrayDataSource<T> extends DataSource<T> {
constructor(private _data: T[] | ReadonlyArray<T> | Observable<T[] | ReadonlyArray<T>>) {
super();
Expand Down
47 changes: 47 additions & 0 deletions src/cdk/collections/differ-data-source.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {DataSource} from './data-source';
import {IterableDiffer, IterableDiffers, TrackByFunction} from '@angular/core';
import {Subject, Observable} from 'rxjs';

/** DataSource wrapper for an iterable whose value might change. Emits when changes are detected. */
export class DifferDataSource<T> extends DataSource<T> {
private _differ: IterableDiffer<T>;
private _changes = new Subject<T[]>();

constructor(
private _differs: IterableDiffers,
private _iterable: T[],
trackBy?: TrackByFunction<T>) {

super();
this._differ = _differs.find(_iterable).create(trackBy);
}

connect(): Observable<T[] | ReadonlyArray<T>> {
return this._changes;
}

disconnect() {
this._changes.complete();
}

/** Checks the array for changes. */
doCheck() {
if (this._differ.diff(this._iterable)) {
this._changes.next(this._iterable);
}
}

/** Switches the `trackBy` function of the data source. */
switchTrackBy(trackBy?: TrackByFunction<T>) {
this._differ = this._differs.find(this._iterable).create(trackBy);
this.doCheck();
}
}
1 change: 1 addition & 0 deletions src/cdk/collections/public-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

export * from './array-data-source';
export * from './differ-data-source';
export * from './collection-viewer';
export * from './data-source';
export * from './dispose-view-repeater-strategy';
Expand Down
32 changes: 29 additions & 3 deletions src/cdk/scrolling/virtual-for-of.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
ArrayDataSource,
CollectionViewer,
DataSource,
DifferDataSource,
ListRange,
isDataSource,
_RecycleViewRepeaterStrategy,
Expand Down Expand Up @@ -95,20 +96,33 @@ export class CdkVirtualForOf<T> implements
/** Subject that emits when a new DataSource instance is given. */
private _dataSourceChanges = new Subject<DataSource<T>>();

/**
* 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.


/** The DataSource to display. */
@Input()
get cdkVirtualForOf(): DataSource<T> | Observable<T[]> | NgIterable<T> | null | undefined {
return this._cdkVirtualForOf;
}
set cdkVirtualForOf(value: DataSource<T> | Observable<T[]> | NgIterable<T> | null | undefined) {
this._cdkVirtualForOf = value;

let dataSource: DataSource<T>;

if (isDataSource(value)) {
this._dataSourceChanges.next(value);
dataSource = value;
} else if (Array.isArray(value)) {
this._differDataSource = dataSource =
new DifferDataSource(this._differs, value, this.cdkVirtualForTrackBy);
} else {
// If value is an an NgIterable, convert it to an array.
this._dataSourceChanges.next(new ArrayDataSource<T>(
isObservable(value) ? value : Array.from(value || [])));
dataSource = new ArrayDataSource<T>(isObservable(value) ? value : Array.from(value || []));
}

this._dataSourceChanges.next(dataSource);
}

_cdkVirtualForOf: DataSource<T> | Observable<T[]> | NgIterable<T> | null | undefined;
Expand All @@ -126,6 +140,10 @@ export class CdkVirtualForOf<T> implements
this._cdkVirtualForTrackBy = fn ?
(index, item) => fn(index + (this._renderedRange ? this._renderedRange.start : 0), item) :
undefined;

if (this._differDataSource) {
this._differDataSource.switchTrackBy(this._cdkVirtualForTrackBy);
}
}
private _cdkVirtualForTrackBy: TrackByFunction<T> | undefined;

Expand Down Expand Up @@ -255,6 +273,10 @@ export class CdkVirtualForOf<T> implements
}

ngDoCheck() {
if (this._differDataSource) {
this._differDataSource.doCheck();
}

if (this._differ && this._needsUpdate) {
// TODO(mmalerba): We should differentiate needs update due to scrolling and a new portion of
// this list being rendered (can use simpler algorithm) vs needs update due to data actually
Expand Down Expand Up @@ -303,6 +325,10 @@ export class CdkVirtualForOf<T> implements

if (oldDs) {
oldDs.disconnect(this);

if (oldDs === this._differDataSource) {
this._differDataSource = null;
}
}

this._needsUpdate = true;
Expand Down
22 changes: 22 additions & 0 deletions src/cdk/scrolling/virtual-scroll-viewport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,28 @@ describe('CdkVirtualScrollViewport', () => {
expect(dataSource.disconnect).toHaveBeenCalled();
}));

it('should work if the iterable is mutated', fakeAsync(() => {
testComponent.items = [];
finishInit(fixture);

expect(viewport.getRenderedRange())
.toEqual({start: 0, end: 0}, 'no items should be rendered');

testComponent.items.push(1, 2, 3);
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 0, end: 3}, 'newly emitted items should be rendered');

testComponent.items.pop();
fixture.detectChanges();
flush();

expect(viewport.getRenderedRange())
.toEqual({start: 0, end: 2}, 'last item to be removed');
}));

it('should trackBy value by default', fakeAsync(() => {
testComponent.items = [];
spyOn(testComponent.virtualForOf._viewContainerRef, 'detach').and.callThrough();
Expand Down
8 changes: 8 additions & 0 deletions tools/public_api_guard/cdk/collections.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ export declare abstract class DataSource<T> {
abstract disconnect(collectionViewer: CollectionViewer): void;
}

export declare class DifferDataSource<T> extends DataSource<T> {
constructor(_differs: IterableDiffers, _iterable: T[], trackBy?: TrackByFunction<T>);
connect(): Observable<T[] | ReadonlyArray<T>>;
disconnect(): void;
doCheck(): void;
switchTrackBy(trackBy?: TrackByFunction<T>): void;
}

export declare function getMultipleValuesInSingleSelectionError(): Error;

export declare function isDataSource(value: any): value is DataSource<any>;
Expand Down