From 6ae68f39b939954a93d8a6301213eb3287c3eaae Mon Sep 17 00:00:00 2001 From: Gerald Monaco Date: Tue, 10 Oct 2023 17:52:40 +0000 Subject: [PATCH] refactor(core): run internal work outside of public afterRender phases (#52145) Public afterRender phases have specific API guarantees which can be invalidated if the internal framework is implemented using them. Instead, the framework should use dedicated internal functions. PR Close #52145 --- .../core/src/core_render3_private_export.ts | 2 +- packages/core/src/defer/dom_triggers.ts | 52 +++++++------ .../core/src/render3/after_render_hooks.ts | 45 +++++++++++ .../test/acceptance/after_render_hook_spec.ts | 78 ++++++++++++++++++- 4 files changed, 150 insertions(+), 27 deletions(-) diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index 151b85702b6..a3a2b1ffb15 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/packages/core/src/core_render3_private_export.ts @@ -313,7 +313,7 @@ export { export { noSideEffects as ɵnoSideEffects, } from './util/closure'; -export { AfterRenderEventManager as ɵAfterRenderEventManager } from './render3/after_render_hooks'; +export { AfterRenderEventManager as ɵAfterRenderEventManager, internalAfterNextRender as ɵinternalAfterNextRender } from './render3/after_render_hooks'; export {depsTracker as ɵdepsTracker, USE_RUNTIME_DEPS_TRACKER_FOR_JIT as ɵUSE_RUNTIME_DEPS_TRACKER_FOR_JIT} from './render3/deps_tracker/deps_tracker'; export {generateStandaloneInDeclarationsError as ɵgenerateStandaloneInDeclarationsError} from './render3/jit/module'; export {getAsyncClassMetadata as ɵgetAsyncClassMetadata} from './render3/metadata'; diff --git a/packages/core/src/defer/dom_triggers.ts b/packages/core/src/defer/dom_triggers.ts index 6f62eb9e90a..2c450bbc043 100644 --- a/packages/core/src/defer/dom_triggers.ts +++ b/packages/core/src/defer/dom_triggers.ts @@ -7,11 +7,12 @@ */ import type {Injector} from '../di'; -import {afterRender} from '../render3/after_render_hooks'; +import {internalAfterNextRender} from '../render3/after_render_hooks'; import {assertLContainer, assertLView} from '../render3/assert'; import {CONTAINER_HEADER_OFFSET} from '../render3/interfaces/container'; import {TNode} from '../render3/interfaces/node'; -import {FLAGS, HEADER_OFFSET, INJECTOR, LView, LViewFlags} from '../render3/interfaces/view'; +import {isDestroyed} from '../render3/interfaces/type_checks'; +import {HEADER_OFFSET, INJECTOR, LView} from '../render3/interfaces/view'; import {getNativeByIndex, removeLViewOnDestroy, storeLViewOnDestroy, walkUpViews} from '../render3/util/view_utils'; import {assertElement, assertEqual} from '../util/assert'; import {NgZone} from '../zone'; @@ -83,13 +84,12 @@ export function onInteraction( entry = new DeferEventEntry(); interactionTriggers.set(trigger, entry); - // Ensure that the handler runs in the NgZone since it gets - // registered in `afterRender` which runs outside. - injector.get(NgZone).run(() => { - for (const name of interactionEventNames) { - trigger.addEventListener(name, entry!.listener, eventListenerOptions); - } - }); + // Ensure that the handler runs in the NgZone + ngDevMode && NgZone.assertInAngularZone(); + + for (const name of interactionEventNames) { + trigger.addEventListener(name, entry!.listener, eventListenerOptions); + } } entry.callbacks.add(callback); @@ -122,13 +122,13 @@ export function onHover( if (!entry) { entry = new DeferEventEntry(); hoverTriggers.set(trigger, entry); - // Ensure that the handler runs in the NgZone since it gets - // registered in `afterRender` which runs outside. - injector.get(NgZone).run(() => { - for (const name of hoverEventNames) { - trigger.addEventListener(name, entry!.listener, eventListenerOptions); - } - }); + + // Ensure that the handler runs in the NgZone + ngDevMode && NgZone.assertInAngularZone(); + + for (const name of hoverEventNames) { + trigger.addEventListener(name, entry!.listener, eventListenerOptions); + } } entry.callbacks.add(callback); @@ -262,31 +262,31 @@ export function registerDomTrigger( registerFn: (element: Element, callback: VoidFunction, injector: Injector) => VoidFunction, callback: VoidFunction) { const injector = initialLView[INJECTOR]!; + function pollDomTrigger() { + // If the initial view was destroyed, we don't need to do anything. + if (isDestroyed(initialLView)) { + return; + } - // Assumption: the `afterRender` reference should be destroyed - // automatically so we don't need to keep track of it. - const afterRenderRef = afterRender(() => { const lDetails = getLDeferBlockDetails(initialLView, tNode); const renderedState = lDetails[DEFER_BLOCK_STATE]; // If the block was loaded before the trigger was resolved, we don't need to do anything. if (renderedState !== DeferBlockInternalState.Initial && renderedState !== DeferBlockState.Placeholder) { - afterRenderRef.destroy(); return; } const triggerLView = getTriggerLView(initialLView, tNode, walkUpTimes); // Keep polling until we resolve the trigger's LView. - // `afterRender` should stop automatically if the view is destroyed. if (!triggerLView) { + internalAfterNextRender(pollDomTrigger, {injector}); return; } // It's possible that the trigger's view was destroyed before we resolved the trigger element. - if (triggerLView[FLAGS] & LViewFlags.Destroyed) { - afterRenderRef.destroy(); + if (isDestroyed(triggerLView)) { return; } @@ -301,7 +301,6 @@ export function registerDomTrigger( cleanup(); }, injector); - afterRenderRef.destroy(); storeLViewOnDestroy(triggerLView, cleanup); // Since the trigger and deferred block might be in different @@ -309,5 +308,8 @@ export function registerDomTrigger( if (initialLView !== triggerLView) { storeLViewOnDestroy(initialLView, cleanup); } - }, {injector}); + } + + // Begin polling for the trigger. + internalAfterNextRender(pollDomTrigger, {injector}); } diff --git a/packages/core/src/render3/after_render_hooks.ts b/packages/core/src/render3/after_render_hooks.ts index f7b48422deb..4662c4b711a 100644 --- a/packages/core/src/render3/after_render_hooks.ts +++ b/packages/core/src/render3/after_render_hooks.ts @@ -118,6 +118,40 @@ export interface AfterRenderRef { destroy(): void; } +/** + * Options passed to `internalAfterNextRender`. + */ +export interface InternalAfterNextRenderOptions { + /** + * The `Injector` to use during creation. + * + * If this is not provided, the current injection context will be used instead (via `inject`). + */ + injector?: Injector; +} + +/** + * Register a callback to run once before any userspace `afterRender` or + * `afterNextRender` callbacks. + * + * This function should almost always be used instead of `afterRender` or + * `afterNextRender` for implementing framework functionality. Consider: + * + * 1.) `AfterRenderPhase.EarlyRead` is intended to be used for implementing + * custom layout. If the framework itself mutates the DOM after *any* + * `AfterRenderPhase.EarlyRead` callbacks are run, the phase can no + * longer reliably serve its purpose. + * + * 2.) Importing `afterRender` in the framework can reduce the ability for it + * to be tree-shaken, and the framework shouldn't need much of the behavior. + */ +export function internalAfterNextRender( + callback: VoidFunction, options?: InternalAfterNextRenderOptions) { + const injector = options?.injector ?? inject(Injector); + const afterRenderEventManager = injector.get(AfterRenderEventManager); + afterRenderEventManager.internalCallbacks.push(callback); +} + /** * Register a callback to be invoked each time the application * finishes rendering. @@ -398,6 +432,9 @@ export class AfterRenderEventManager { /* @internal */ handler: AfterRenderCallbackHandler|null = null; + /* @internal */ + internalCallbacks: VoidFunction[] = []; + /** * Mark the beginning of a render operation (i.e. CD cycle). * Throws if called while executing callbacks. @@ -416,6 +453,13 @@ export class AfterRenderEventManager { this.renderDepth--; if (this.renderDepth === 0) { + // Note: internal callbacks power `internalAfterNextRender`. Since internal callbacks + // are fairly trivial, they are kept separate so that `AfterRenderCallbackHandlerImpl` + // can still be tree-shaken unless used by the application. + for (const callback of this.internalCallbacks) { + callback(); + } + this.internalCallbacks.length = 0; this.handler?.execute(); } } @@ -423,6 +467,7 @@ export class AfterRenderEventManager { ngOnDestroy() { this.handler?.destroy(); this.handler = null; + this.internalCallbacks.length = 0; } /** @nocollapse */ diff --git a/packages/core/test/acceptance/after_render_hook_spec.ts b/packages/core/test/acceptance/after_render_hook_spec.ts index 15ad27d943e..bf8971c7a21 100644 --- a/packages/core/test/acceptance/after_render_hook_spec.ts +++ b/packages/core/test/acceptance/after_render_hook_spec.ts @@ -7,7 +7,7 @@ */ import {PLATFORM_BROWSER_ID, PLATFORM_SERVER_ID} from '@angular/common/src/platform_id'; -import {afterNextRender, afterRender, AfterRenderPhase, AfterRenderRef, ChangeDetectorRef, Component, computed, effect, ErrorHandler, inject, Injector, NgZone, PLATFORM_ID, ViewContainerRef} from '@angular/core'; +import {afterNextRender, afterRender, AfterRenderPhase, AfterRenderRef, ChangeDetectorRef, Component, computed, effect, ErrorHandler, inject, Injector, NgZone, PLATFORM_ID, ViewContainerRef, ɵinternalAfterNextRender as internalAfterNextRender} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('after render hooks', () => { @@ -16,6 +16,82 @@ describe('after render hooks', () => { providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}] }; + describe('internalAfterNextRender', () => { + it('should run with the expected timing', () => { + const log: string[] = []; + + @Component({selector: 'comp'}) + class Comp { + constructor() { + // Helper to register into each phase + function forEachPhase(fn: (phase: AfterRenderPhase) => void) { + for (const phase in AfterRenderPhase) { + const val = AfterRenderPhase[phase]; + if (typeof val === 'number') { + fn(val); + } + } + } + + internalAfterNextRender(() => { + log.push('internalAfterNextRender #1'); + }); + + forEachPhase(phase => afterRender(() => { + log.push(`afterRender (${AfterRenderPhase[phase]})`); + }, {phase})); + + internalAfterNextRender(() => { + log.push('internalAfterNextRender #2'); + }); + + forEachPhase(phase => afterNextRender(() => { + log.push(`afterNextRender (${AfterRenderPhase[phase]})`); + }, {phase})); + + internalAfterNextRender(() => { + log.push('internalAfterNextRender #3'); + }); + } + } + + TestBed.configureTestingModule({ + declarations: [Comp], + ...COMMON_CONFIGURATION, + }); + const fixture = TestBed.createComponent(Comp); + + // It hasn't run at all + expect(log).toEqual([]); + + // Running change detection once + fixture.detectChanges(); + expect(log).toEqual([ + 'internalAfterNextRender #1', + 'internalAfterNextRender #2', + 'internalAfterNextRender #3', + 'afterRender (EarlyRead)', + 'afterNextRender (EarlyRead)', + 'afterRender (Write)', + 'afterNextRender (Write)', + 'afterRender (MixedReadWrite)', + 'afterNextRender (MixedReadWrite)', + 'afterRender (Read)', + 'afterNextRender (Read)', + ]); + + // Running change detection again + log.length = 0; + fixture.detectChanges(); + expect(log).toEqual([ + 'afterRender (EarlyRead)', + 'afterRender (Write)', + 'afterRender (MixedReadWrite)', + 'afterRender (Read)', + ]); + }); + }); + describe('afterRender', () => { it('should run with the correct timing', () => { @Component({selector: 'dynamic-comp'})