From da6970d2cbfaa032f674fabb7bb4a98903dbecf7 Mon Sep 17 00:00:00 2001 From: arturovt Date: Tue, 1 Oct 2024 18:31:03 +0300 Subject: [PATCH] refactor(docs-infra): allow table of contents to be GCed (#58034) This commit updates the table of contents functionality to clean up correctly whenever the user navigates to other pages and nodes are removed from the DOM. Currently, calling `renderComponent` with the `TableOfContents` keeps creating a new table of contents component without removing the previous one, as they are created manually. This leads to memory leaks because the components cannot be collected properly, even if the user navigates to the home page where there is no TOC component. PR Close #58034 --- .../table-of-contents.component.ts | 13 +++++-- .../docs-viewer/docs-viewer.component.ts | 16 ++++++--- ...ble-of-contents-scroll-spy.service.spec.ts | 11 +++--- .../table-of-contents-scroll-spy.service.ts | 36 +++++++++++++------ 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/adev/shared-docs/components/table-of-contents/table-of-contents.component.ts b/adev/shared-docs/components/table-of-contents/table-of-contents.component.ts index e2ef35e83f8..f9e36039c36 100644 --- a/adev/shared-docs/components/table-of-contents/table-of-contents.component.ts +++ b/adev/shared-docs/components/table-of-contents/table-of-contents.component.ts @@ -6,7 +6,14 @@ * found in the LICENSE file at https://angular.dev/license */ -import {ChangeDetectionStrategy, Component, Input, computed, inject} from '@angular/core'; +import { + ChangeDetectionStrategy, + Component, + DestroyRef, + Input, + computed, + inject, +} from '@angular/core'; import {RouterLink} from '@angular/router'; import {TableOfContentsLevel} from '../../interfaces/index'; import {TableOfContentsLoader} from '../../services/table-of-contents-loader.service'; @@ -27,6 +34,8 @@ export class TableOfContents { private readonly scrollSpy = inject(TableOfContentsScrollSpy); private readonly tableOfContentsLoader = inject(TableOfContentsLoader); + private readonly destroyRef = inject(DestroyRef); + tableOfContentItems = this.tableOfContentsLoader.tableOfContentItems; activeItemId = this.scrollSpy.activeItemId; @@ -35,7 +44,7 @@ export class TableOfContents { ngAfterViewInit() { this.tableOfContentsLoader.buildTableOfContent(this.contentSourceElement); - this.scrollSpy.startListeningToScroll(this.contentSourceElement); + this.scrollSpy.startListeningToScroll(this.contentSourceElement, this.destroyRef); } scrollToTop(): void { diff --git a/adev/shared-docs/components/viewers/docs-viewer/docs-viewer.component.ts b/adev/shared-docs/components/viewers/docs-viewer/docs-viewer.component.ts index 4b88f650114..e99843e64d5 100644 --- a/adev/shared-docs/components/viewers/docs-viewer/docs-viewer.component.ts +++ b/adev/shared-docs/components/viewers/docs-viewer/docs-viewer.component.ts @@ -75,7 +75,6 @@ export class DocViewer implements OnChanges { private readonly elementRef = inject(ElementRef); private readonly location = inject(Location); private readonly navigationState = inject(NavigationState); - private readonly platformId = inject(PLATFORM_ID); private readonly router = inject(Router); private readonly viewContainer = inject(ViewContainerRef); private readonly environmentInjector = inject(EnvironmentInjector); @@ -86,6 +85,8 @@ export class DocViewer implements OnChanges { private animateContent = false; private readonly pendingTasks = inject(PendingTasks); + private readonly isBrowser = isPlatformBrowser(inject(PLATFORM_ID)); + private countOfExamples = 0; async ngOnChanges(changes: SimpleChanges): Promise { @@ -97,11 +98,10 @@ export class DocViewer implements OnChanges { } async renderContentsAndRunClientSetup(content?: string): Promise { - const isBrowser = isPlatformBrowser(this.platformId); const contentContainer = this.elementRef.nativeElement; if (content) { - if (isBrowser && !(this.document as any).startViewTransition) { + if (this.isBrowser && !(this.document as any).startViewTransition) { // Apply a special class to the host node to trigger animation. // Note: when a page is hydrated, the `content` would be empty, // so we don't trigger an animation to avoid a content flickering @@ -112,7 +112,7 @@ export class DocViewer implements OnChanges { contentContainer.innerHTML = content; } - if (isBrowser) { + if (this.isBrowser) { // First we setup event listeners on the HTML we just loaded. // We want to do this before things like the example viewers are loaded. this.setupAnchorListeners(contentContainer); @@ -301,6 +301,14 @@ export class DocViewer implements OnChanges { // during SSG. this.appRef.attachView(componentRef.hostView); + // This is wrapped with `isBrowser` in for hydration purposes. + if (this.isBrowser) { + // The `docs-viewer` may be rendered multiple times when navigating + // between pages, which will create new components that need to be + // destroyed for gradual cleanup. + this.destroyRef.onDestroy(() => componentRef.destroy()); + } + return componentRef; } diff --git a/adev/shared-docs/services/table-of-contents-scroll-spy.service.spec.ts b/adev/shared-docs/services/table-of-contents-scroll-spy.service.spec.ts index d4ec27cb0de..6e045a20c1f 100644 --- a/adev/shared-docs/services/table-of-contents-scroll-spy.service.spec.ts +++ b/adev/shared-docs/services/table-of-contents-scroll-spy.service.spec.ts @@ -6,17 +6,19 @@ * found in the LICENSE file at https://angular.dev/license */ +import {DestroyRef} from '@angular/core'; +import {DOCUMENT, ViewportScroller} from '@angular/common'; import {TestBed, discardPeriodicTasks, fakeAsync, tick} from '@angular/core/testing'; import {WINDOW} from '../providers'; import {TableOfContentsLoader} from './table-of-contents-loader.service'; import {SCROLL_EVENT_DELAY, TableOfContentsScrollSpy} from './table-of-contents-scroll-spy.service'; -import {DOCUMENT, ViewportScroller} from '@angular/common'; import {TableOfContentsLevel} from '../interfaces'; describe('TableOfContentsScrollSpy', () => { let service: TableOfContentsScrollSpy; + let destroyRef: DestroyRef; const fakeWindow = { addEventListener: () => {}, removeEventListener: () => {}, @@ -68,6 +70,7 @@ describe('TableOfContentsScrollSpy', () => { const tableOfContentsLoaderSpy = TestBed.inject(TableOfContentsLoader); tableOfContentsLoaderSpy.tableOfContentItems.set(fakeToCItems); service = TestBed.inject(TableOfContentsScrollSpy); + destroyRef = TestBed.inject(DestroyRef); }); it('should be created', () => { @@ -94,7 +97,7 @@ describe('TableOfContentsScrollSpy', () => { const scrollableContainer = doc; const setActiveItemIdSpy = spyOn(service as any, 'setActiveItemId'); - service.startListeningToScroll(doc.querySelector('fake-selector')); + service.startListeningToScroll(doc.querySelector('fake-selector'), destroyRef); scrollableContainer.dispatchEvent(new Event('scroll')); tick(SCROLL_EVENT_DELAY - 2); @@ -118,7 +121,7 @@ describe('TableOfContentsScrollSpy', () => { const doc = TestBed.inject(DOCUMENT); const scrollableContainer = doc; - service.startListeningToScroll(doc.querySelector('fake-selector')); + service.startListeningToScroll(doc.querySelector('fake-selector'), destroyRef); fakeWindow.scrollY = 1238; scrollableContainer.dispatchEvent(new Event('scroll')); @@ -133,7 +136,7 @@ describe('TableOfContentsScrollSpy', () => { const doc = TestBed.inject(DOCUMENT); const scrollableContainer = doc; - service.startListeningToScroll(doc.querySelector('fake-selector')); + service.startListeningToScroll(doc.querySelector('fake-selector'), destroyRef); fakeWindow.scrollY = 99; scrollableContainer.dispatchEvent(new Event('scroll')); diff --git a/adev/shared-docs/services/table-of-contents-scroll-spy.service.ts b/adev/shared-docs/services/table-of-contents-scroll-spy.service.ts index 17f66d82ab1..040d0d7ee12 100644 --- a/adev/shared-docs/services/table-of-contents-scroll-spy.service.ts +++ b/adev/shared-docs/services/table-of-contents-scroll-spy.service.ts @@ -29,7 +29,6 @@ export const SCROLL_FINISH_DELAY = SCROLL_EVENT_DELAY * 2; // The service is responsible for listening for scrolling and resizing, // thanks to which it sets the active item in the Table of contents export class TableOfContentsScrollSpy { - private readonly destroyRef = inject(DestroyRef); private readonly tableOfContentsLoader = inject(TableOfContentsLoader); private readonly document = inject(DOCUMENT); private readonly window = inject(WINDOW); @@ -41,12 +40,25 @@ export class TableOfContentsScrollSpy { activeItemId = signal(null); scrollbarThumbOnTop = signal(true); - startListeningToScroll(contentSourceElement: HTMLElement | null): void { + startListeningToScroll( + contentSourceElement: HTMLElement | null, + // `destroyRef` is required because the caller may invoke `startListeningToScroll` + // multiple times. Without it, previous event listeners would not be disposed of, + // leading to the accumulation of new event listeners. + destroyRef: DestroyRef, + ) { this.contentSourceElement = contentSourceElement; this.lastContentWidth = this.getContentWidth(); - this.setScrollEventHandlers(); - this.setResizeEventHandlers(); + this.setScrollEventHandlers(destroyRef); + this.setResizeEventHandlers(destroyRef); + + destroyRef.onDestroy(() => { + // We also need to clean up the source element once the view that calls + // `startListeningToScroll` is destroyed, as this will keep a reference + // to an element that has been removed from the DOM. + this.contentSourceElement = null; + }); } scrollToTop(): void { @@ -72,9 +84,9 @@ export class TableOfContentsScrollSpy { } // After window resize, we should update top value of each table content item - private setResizeEventHandlers() { + private setResizeEventHandlers(destroyRef: DestroyRef) { fromEvent(this.window, 'resize') - .pipe(debounceTime(RESIZE_EVENT_DELAY), takeUntilDestroyed(this.destroyRef), startWith()) + .pipe(debounceTime(RESIZE_EVENT_DELAY), takeUntilDestroyed(destroyRef), startWith()) .subscribe(() => { this.updateHeadingsTopAfterResize(); }); @@ -83,14 +95,16 @@ export class TableOfContentsScrollSpy { // assets (fonts, images) are loaded. They can (and will) change the y-position of the headings. const docsViewer = this.document.querySelector('docs-viewer'); if (docsViewer) { - afterNextRender( + const ref = afterNextRender( () => { const resizeObserver = new ResizeObserver(() => this.updateHeadingsTopAfterResize()); resizeObserver.observe(docsViewer); - this.destroyRef.onDestroy(() => resizeObserver.disconnect()); + destroyRef.onDestroy(() => resizeObserver.disconnect()); }, - {injector: this.injector}, + {injector: this.injector, manualCleanup: true}, ); + + destroyRef.onDestroy(() => ref.destroy()); } } @@ -104,10 +118,10 @@ export class TableOfContentsScrollSpy { } } - private setScrollEventHandlers(): void { + private setScrollEventHandlers(destroyRef: DestroyRef): void { const scroll$ = fromEvent(this.document, 'scroll').pipe( auditTime(SCROLL_EVENT_DELAY), - takeUntilDestroyed(this.destroyRef), + takeUntilDestroyed(destroyRef), ); scroll$.subscribe(() => this.setActiveItemId());