refactor(core): restructure AfterRenderManager to connect related phases (angular#57453)

The `afterRender` infrastructure was first implemented around the idea of
independent, singular hooks. It was later updated to support a spec of
multiple hooks that pass values from one to another as they execute, but the
implementation still worked in terms of singular hooks under the hood. This
creates a number of maintenance issues, and a few bugs. For example, when
one hook fails, further hooks in the pipeline should no longer execute, but
this was hard to ensure under the old design.

This refactoring restructures `afterRender` infrastructure significantly to
introduce the concept of a "sequence", a collection of hooks of different
phases that execute together. Overall, the implementation is simplified
while making it more resilient to issues and future use cases, such as the
upcoming `afterRenderEffect`.

As part of this refactoring, the `internalAfterNextRender` concept is
removed, as well as the unused `queueStateUpdate` concept which used it.

PR Close angular#57453

Author: alxhub

packages/core/src/application/application_ref.ts

@@ -31,7 +31,7 @@ import {NgModuleRef} from '../linker/ng_module_factory';
 import {ViewRef} from '../linker/view_ref';
 import {PendingTasks} from '../pending_tasks';
 import {RendererFactory2} from '../render/api';
-import {AfterRenderEventManager} from '../render3/after_render_hooks';
+import {AfterRenderManager} from '../render3/after_render/manager';
 import {ComponentFactory as R3ComponentFactory} from '../render3/component_ref';
 import {isStandalone} from '../render3/definition';
 import {ChangeDetectionMode, detectChangesInternal} from '../render3/instructions/change_detection';
@@ -308,7 +308,7 @@ export class ApplicationRef {
   /** @internal */
   _views: InternalViewRef<unknown>[] = [];
   private readonly internalErrorHandler = inject(INTERNAL_APPLICATION_ERROR_HANDLER);
-  private readonly afterRenderEffectManager = inject(AfterRenderEventManager);
+  private readonly afterRenderManager = inject(AfterRenderManager);
   private readonly zonelessEnabled = inject(ZONELESS_ENABLED);
 
   /**
@@ -676,6 +676,13 @@ export class ApplicationRef {
       // flag. We clear the flag here because, for backwards compatibility, `markForCheck()`
       // during view checking doesn't cause the view to be re-checked.
       this.dirtyFlags &= ~ApplicationRefDirtyFlags.ViewTreeCheck;
+
+      // Check if any views are still dirty after checking and we need to loop back.
+      this.syncDirtyFlagsWithViews();
+      if (this.dirtyFlags & ApplicationRefDirtyFlags.ViewTreeAny) {
+        // If any views are still dirty after checking, loop back before running render hooks.
+        return;
+      }
     } else {
       // If we skipped refreshing views above, there might still be unflushed animations
       // because we never called `detectChangesInternal` on the views.
@@ -686,32 +693,32 @@ export class ApplicationRef {
     // Even if there were no dirty views, afterRender hooks might still be dirty.
     if (this.dirtyFlags & ApplicationRefDirtyFlags.AfterRender) {
       this.dirtyFlags &= ~ApplicationRefDirtyFlags.AfterRender;
+      this.afterRenderManager.execute();
 
-      this.afterRenderEffectManager.executeInternalCallbacks();
-
-      // If we have a newly dirty view after running internal callbacks, recheck the views again
-      // before running user-provided callbacks
-      if (this.allViews.some(({_lView}) => requiresRefreshOrTraversal(_lView))) {
-        // Should be a no-op, but just in case:
-        this.dirtyFlags |= ApplicationRefDirtyFlags.ViewTreeTraversal;
-        return;
-      }
-
-      this.afterRenderEffectManager.execute();
-
-      if (this.dirtyFlags & ApplicationRefDirtyFlags.AfterRender) {
-        // If an afterRender hook schedules new afterRender hooks, we don't immediately loop, but
-        // instead queue them to run in the next synchronization pass, whenever that is. Therefore
-        // we clear the `AfterRender` bit here, but add it to `deferredDirtyFlags` to be applied
-        // on the next pass.
-        this.dirtyFlags &= ~ApplicationRefDirtyFlags.AfterRender;
-        this.deferredDirtyFlags |= ApplicationRefDirtyFlags.AfterRender;
-      }
+      // afterRender hooks might influence dirty flags.
     }
+    this.syncDirtyFlagsWithViews();
+  }
 
+  /**
+   * Checks `allViews` for views which require refresh/traversal, and updates `dirtyFlags`
+   * accordingly, with two potential behaviors:
+   *
+   * 1. If any of our views require updating, then this adds the `ViewTreeTraversal` dirty flag.
+   *    This _should_ be a no-op, since the scheduler should've added the flag at the same time the
+   *    view was marked as needing updating.
+   *
+   *    TODO(alxhub): figure out if this behavior is still needed for edge cases.
+   *
+   * 2. If none of our views require updating, then clear the view-related `dirtyFlag`s. This
+   *    happens when the scheduler is notified of a view becoming dirty, but the view itself isn't
+   *    reachable through traversal from our roots (e.g. it's detached from the CD tree).
+   */
+  private syncDirtyFlagsWithViews(): void {
     if (this.allViews.some(({_lView}) => requiresRefreshOrTraversal(_lView))) {
       // If after running all afterRender callbacks new views are dirty, ensure we loop back.
       this.dirtyFlags |= ApplicationRefDirtyFlags.ViewTreeTraversal;
+      return;
     } else {
       // Even though this flag may be set, none of _our_ views require traversal, and so the
       // `ApplicationRef` doesn't require any repeated checking.

packages/core/src/change_detection/scheduling/zoneless_scheduling.ts

@@ -29,7 +29,8 @@ export const enum NotificationSource {
   // The following notifications do not require views to be refreshed
   // but we should execute render hooks:
   // Render hooks are guaranteed to execute with the schedulers timing.
-  NewRenderHook,
+  RenderHook,
+  DeferredRenderHook,
   // Views might be created outside and manipulated in ways that
   // we cannot be aware of. When a view is attached, Angular now "knows"
   // about it and we now know that DOM might have changed (and we should

packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts

@@ -135,9 +135,16 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
         this.appRef.dirtyFlags |= ApplicationRefDirtyFlags.ViewTreeCheck;
         break;
       }
+      case NotificationSource.DeferredRenderHook: {
+        // Render hooks are "deferred" when they're triggered from other render hooks. Using the
+        // deferred dirty flags ensures that adding new hooks doesn't automatically trigger a loop
+        // inside tick().
+        this.appRef.deferredDirtyFlags |= ApplicationRefDirtyFlags.AfterRender;
+        break;
+      }
       case NotificationSource.ViewDetachedFromDOM:
       case NotificationSource.ViewAttached:
-      case NotificationSource.NewRenderHook:
+      case NotificationSource.RenderHook:
       case NotificationSource.AsyncAnimationsLoaded:
       default: {
         // These notifications only schedule a tick but do not change whether we should refresh

packages/core/src/core.ts

@@ -98,14 +98,13 @@ export {
 } from './render3/ng_module_ref';
 export {createComponent, reflectComponentType, ComponentMirror} from './render3/component';
 export {isStandalone} from './render3/definition';
+export {AfterRenderPhase, AfterRenderRef} from './render3/after_render/api';
 export {
-  AfterRenderRef,
   AfterRenderOptions,
-  AfterRenderPhase,
   afterRender,
   afterNextRender,
   ɵFirstAvailable,
-} from './render3/after_render_hooks';
+} from './render3/after_render/hooks';
 export {ApplicationConfig, mergeApplicationConfig} from './application/application_config';
 export {makeStateKey, StateKey, TransferState} from './transfer_state';
 export {booleanAttribute, numberAttribute} from './util/coercion';

packages/core/src/core_private_export.ts

@@ -112,7 +112,6 @@ export {
   ProviderRecord as ɵProviderRecord,
   setInjectorProfilerContext as ɵsetInjectorProfilerContext,
 } from './render3/debug/injector_profiler';
-export {queueStateUpdate as ɵqueueStateUpdate} from './render3/queue_state_update';
 export {
   allowSanitizationBypassAndThrow as ɵallowSanitizationBypassAndThrow,
   BypassType as ɵBypassType,

packages/core/src/core_render3_private_export.ts

@@ -293,10 +293,7 @@ export {
 } from './sanitization/sanitization';
 export {ɵɵvalidateIframeAttribute} from './sanitization/iframe_attrs_validation';
 export {noSideEffects as ɵnoSideEffects} from './util/closure';
-export {
-  AfterRenderEventManager as ɵAfterRenderEventManager,
-  internalAfterNextRender as ɵinternalAfterNextRender,
-} from './render3/after_render_hooks';
+export {AfterRenderManager as ɵAfterRenderManager} from './render3/after_render/manager';
 export {
   depsTracker as ɵdepsTracker,
   USE_RUNTIME_DEPS_TRACKER_FOR_JIT as ɵUSE_RUNTIME_DEPS_TRACKER_FOR_JIT,

packages/core/src/defer/dom_triggers.ts

@@ -6,8 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {afterNextRender} from '../render3/after_render/hooks';
 import type {Injector} from '../di';
-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';
@@ -279,6 +279,7 @@ export function registerDomTrigger(
   type: TriggerType,
 ) {
   const injector = initialLView[INJECTOR]!;
+  const zone = injector.get(NgZone);
   function pollDomTrigger() {
     // If the initial view was destroyed, we don't need to do anything.
     if (isDestroyed(initialLView)) {
@@ -300,7 +301,7 @@ export function registerDomTrigger(
 
     // Keep polling until we resolve the trigger's LView.
     if (!triggerLView) {
-      internalAfterNextRender(pollDomTrigger, {injector});
+      afterNextRender({read: pollDomTrigger}, {injector});
       return;
     }
 
@@ -313,10 +314,14 @@ export function registerDomTrigger(
     const cleanup = registerFn(
       element,
       () => {
-        if (initialLView !== triggerLView) {
-          removeLViewOnDestroy(triggerLView, cleanup);
-        }
-        callback();
+        // `pollDomTrigger` runs outside the zone (because of `afterNextRender`) and registers its
+        // listeners outside the zone, so we jump back into the zone prior to running the callback.
+        zone.run(() => {
+          if (initialLView !== triggerLView) {
+            removeLViewOnDestroy(triggerLView, cleanup);
+          }
+          callback();
+        });
       },
       injector,
     );
@@ -334,5 +339,5 @@ export function registerDomTrigger(
   }
 
   // Begin polling for the trigger.
-  internalAfterNextRender(pollDomTrigger, {injector});
+  afterNextRender({read: pollDomTrigger}, {injector});
 }

packages/core/src/render3/after_render/api.ts

@@ -0,0 +1,84 @@
+/**
+ * @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
+ */
+
+/**
+ * The phase to run an `afterRender` or `afterNextRender` callback in.
+ *
+ * Callbacks in the same phase run in the order they are registered. Phases run in the
+ * following order after each render:
+ *
+ *   1. `AfterRenderPhase.EarlyRead`
+ *   2. `AfterRenderPhase.Write`
+ *   3. `AfterRenderPhase.MixedReadWrite`
+ *   4. `AfterRenderPhase.Read`
+ *
+ * Angular is unable to verify or enforce that phases are used correctly, and instead
+ * relies on each developer to follow the guidelines documented for each value and
+ * carefully choose the appropriate one, refactoring their code if necessary. By doing
+ * so, Angular is better able to minimize the performance degradation associated with
+ * manual DOM access, ensuring the best experience for the end users of your application
+ * or library.
+ *
+ * @deprecated Specify the phase for your callback to run in by passing a spec-object as the first
+ *   parameter to `afterRender` or `afterNextRender` instead of a function.
+ */
+export enum AfterRenderPhase {
+  /**
+   * Use `AfterRenderPhase.EarlyRead` for callbacks that only need to **read** from the
+   * DOM before a subsequent `AfterRenderPhase.Write` callback, for example to perform
+   * custom layout that the browser doesn't natively support. Prefer the
+   * `AfterRenderPhase.EarlyRead` phase if reading can wait until after the write phase.
+   * **Never** write to the DOM in this phase.
+   *
+   * <div class="alert is-important">
+   *
+   * Using this value can degrade performance.
+   * Instead, prefer using built-in browser functionality when possible.
+   *
+   * </div>
+   */
+  EarlyRead,
+
+  /**
+   * Use `AfterRenderPhase.Write` for callbacks that only **write** to the DOM. **Never**
+   * read from the DOM in this phase.
+   */
+  Write,
+
+  /**
+   * Use `AfterRenderPhase.MixedReadWrite` for callbacks that read from or write to the
+   * DOM, that haven't been refactored to use a different phase. **Never** use this phase if
+   * it is possible to divide the work among the other phases instead.
+   *
+   * <div class="alert is-critical">
+   *
+   * Using this value can **significantly** degrade performance.
+   * Instead, prefer dividing work into the appropriate phase callbacks.
+   *
+   * </div>
+   */
+  MixedReadWrite,
+
+  /**
+   * Use `AfterRenderPhase.Read` for callbacks that only **read** from the DOM. **Never**
+   * write to the DOM in this phase.
+   */
+  Read,
+}
+
+/**
+ * A callback that runs after render.
+ *
+ * @developerPreview
+ */
+export interface AfterRenderRef {
+  /**
+   * Shut down the callback, preventing it from being called again.
+   */
+  destroy(): void;
+}