Skip to content

Commit

Permalink
fix(virtual-scroll): not removing view from container if it's outside…
Browse files Browse the repository at this point in the history
… the template cache (angular#13916)

Currently when detaching a view, we check whether it would fit in the cache, and if it doesn't, we destroy it. Since we destroy the view on it's own, the `ViewContainerRef` still has a reference to it, which means that we'll trigger change detection on it the next time the data changes. These changes switch to destroying the view through the view container.

Fixes angular#13901.
  • Loading branch information
crisbeto authored and atscott committed Nov 5, 2018
1 parent ba004e1 commit 53080e5
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
11 changes: 10 additions & 1 deletion src/cdk/scrolling/virtual-for-of.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,16 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
if (this._templateCache.length < this.cdkVirtualForTemplateCacheSize) {
this._templateCache.push(view);
} else {
view.destroy();
const index = this._viewContainerRef.indexOf(view);

// It's very unlikely that the index will ever be -1, but just in case,
// destroy the view on its own, otherwise destroy it through the
// container to ensure that all the references are removed.
if (index === -1) {
view.destroy();
} else {
this._viewContainerRef.remove(index);
}
}
}

Expand Down
18 changes: 18 additions & 0 deletions src/cdk/scrolling/virtual-scroll-viewport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,24 @@ describe('CdkVirtualScrollViewport', () => {
finishInit(fixture);
expect(zoneTest).toHaveBeenCalledWith(true);
}));

it('should not throw when disposing of a view that will not fit in the cache', fakeAsync(() => {
finishInit(fixture);
testComponent.items = new Array(200).fill(0);
testComponent.templateCacheSize = 1; // Reduce the cache size to something we can easily hit.
fixture.detectChanges();
flush();

expect(() => {
for (let i = 0; i < 50; i++) {
viewport.scrollToIndex(i);
triggerScroll(viewport);
fixture.detectChanges();
flush();
}
}).not.toThrow();
}));

});

describe('with RTL direction', () => {
Expand Down

0 comments on commit 53080e5

Please sign in to comment.