Skip to content

Commit

Permalink
fix(core): afterRender hooks now only run on ApplicationRef.tick
Browse files Browse the repository at this point in the history
The `afterRender` hooks currently run after `ApplicationRef.tick` but
also run after any call to `ChangeDetectorRef.detectChanges`. This is
problematic because code which uses `afterRender` cannot expect the
component it's registered from to be rendered when the callback
executes. If there is a call to `ChangeDetectorRef.detectChanges` before
the global change detection, that will cause the hooks to run earlier
than expected.

This behavior is somewhat of a blocker for the zoneless project. There
is plenty of application code that do things like `setTimeout(() =>
doSomethingThatExpectsComponentToBeRendered())`, `NgZone.onStable(() =>
...)` or `ApplicationRef.onStable...`. `ApplicationRef.onStable` is a
should likely work similarly, but all of these are really wanting an API
that is `afterRender` with the requirement that the hook runs after the
global render, not an individual CDRef instance.

This change updates the `afterRender` hooks to only run when
`ApplicationRef.tick` happens.

fixes #52429
fixes #53232
  • Loading branch information
atscott committed Dec 14, 2023
1 parent 629343f commit bc3118b
Show file tree
Hide file tree
Showing 8 changed files with 178 additions and 182 deletions.
13 changes: 13 additions & 0 deletions packages/core/src/application/application_ref.ts
Expand Up @@ -29,6 +29,7 @@ import {ComponentFactoryResolver} from '../linker/component_factory_resolver';
import {NgModuleFactory, NgModuleRef} from '../linker/ng_module_factory';
import {ViewRef} from '../linker/view_ref';
import {isComponentResourceResolutionQueueEmpty, resolveComponentResources} from '../metadata/resource_loading';
import {AfterRenderEventManager} from '../render3/after_render_hooks';
import {assertNgModuleType} from '../render3/assert';
import {ComponentFactory as R3ComponentFactory} from '../render3/component_ref';
import {isStandalone} from '../render3/definition';
Expand Down Expand Up @@ -324,6 +325,7 @@ export class ApplicationRef {
_views: InternalViewRef<unknown>[] = [];
private readonly internalErrorHandler = inject(INTERNAL_APPLICATION_ERROR_HANDLER);
private readonly zoneIsStable = inject(ZONE_IS_STABLE_OBSERVABLE);
private readonly afterRenderEffectManager = inject(AfterRenderEventManager, {optional: true});

/**
* Indicates whether this instance was destroyed.
Expand Down Expand Up @@ -561,6 +563,17 @@ export class ApplicationRef {
// Attention: Don't rethrow as it could cancel subscriptions to Observables!
this.internalErrorHandler(e);
} finally {
try {
const callbacksExecuted = this.afterRenderEffectManager?.execute();
if ((typeof ngDevMode === 'undefined' || ngDevMode) && callbacksExecuted) {
for (let view of this._views) {
view.checkNoChanges();
}
}
} catch (e) {
this.internalErrorHandler(e);
}

this._runningTick = false;
}
}
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/errors.ts
Expand Up @@ -30,8 +30,7 @@ export const enum RuntimeErrorCode {
// Change Detection Errors
EXPRESSION_CHANGED_AFTER_CHECKED = -100,
RECURSIVE_APPLICATION_REF_TICK = 101,
RECURSIVE_APPLICATION_RENDER = 102,
INFINITE_CHANGE_DETECTION = 103,
INFINITE_CHANGE_DETECTION = 102,

// Dependency Injection Errors
CYCLIC_DI_DEPENDENCY = -200,
Expand Down
63 changes: 17 additions & 46 deletions packages/core/src/render3/after_render_hooks.ts
Expand Up @@ -349,13 +349,6 @@ class AfterRenderCallback {
* Implements `afterRender` and `afterNextRender` callback handler logic.
*/
interface AfterRenderCallbackHandler {
/**
* Validate that it's safe for a render operation to begin,
* throwing if not. Not guaranteed to be called if a render
* operation is started before handler was registered.
*/
validateBegin(): void;

/**
* Register a new callback.
*/
Expand All @@ -367,9 +360,9 @@ interface AfterRenderCallbackHandler {
unregister(callback: AfterRenderCallback): void;

/**
* Execute callbacks.
* Execute callbacks. Returns `true` if any callbacks were executed.
*/
execute(): void;
execute(): boolean;

/**
* Perform any necessary cleanup.
Expand All @@ -392,16 +385,6 @@ class AfterRenderCallbackHandlerImpl implements AfterRenderCallbackHandler {
};
private deferredCallbacks = new Set<AfterRenderCallback>();

validateBegin(): void {
if (this.executingCallbacks) {
throw new RuntimeError(
RuntimeErrorCode.RECURSIVE_APPLICATION_RENDER,
ngDevMode &&
'A new render operation began before the previous operation ended. ' +
'Did you trigger change detection from afterRender or afterNextRender?');
}
}

register(callback: AfterRenderCallback): void {
// If we're currently running callbacks, new callbacks should be deferred
// until the next render operation.
Expand All @@ -414,10 +397,12 @@ class AfterRenderCallbackHandlerImpl implements AfterRenderCallbackHandler {
this.deferredCallbacks.delete(callback);
}

execute(): void {
execute(): boolean {
let callbacksExecuted = false;
this.executingCallbacks = true;
for (const bucket of Object.values(this.buckets)) {
for (const callback of bucket) {
callbacksExecuted = true;
callback.invoke();
}
}
Expand All @@ -427,6 +412,7 @@ class AfterRenderCallbackHandlerImpl implements AfterRenderCallbackHandler {
this.buckets[callback.phase].add(callback);
}
this.deferredCallbacks.clear();
return callbacksExecuted;
}

destroy(): void {
Expand All @@ -442,41 +428,26 @@ class AfterRenderCallbackHandlerImpl implements AfterRenderCallbackHandler {
* Delegates to an optional `AfterRenderCallbackHandler` for implementation.
*/
export class AfterRenderEventManager {
private renderDepth = 0;

/* @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.
* Executes callbacks. Returns `true` if any callbacks executed.
*/
begin() {
this.handler?.validateBegin();
this.renderDepth++;
}

/**
* Mark the end of a render operation. Callbacks will be
* executed if there are no more pending operations.
*/
end() {
ngDevMode && assertGreaterThan(this.renderDepth, 0, 'renderDepth must be greater than 0');
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();
execute(): boolean {
// 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.
const callbacks = [...this.internalCallbacks];
this.internalCallbacks.length = 0;
for (const callback of callbacks) {
callback();
}
const handlerCallbacksExecuted = this.handler?.execute();
return !!handlerCallbacksExecuted || callbacks.length > 0;
}

ngOnDestroy() {
Expand Down
5 changes: 0 additions & 5 deletions packages/core/src/render3/instructions/change_detection.ts
Expand Up @@ -30,7 +30,6 @@ const MAXIMUM_REFRESH_RERUNS = 100;
export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
const environment = lView[ENVIRONMENT];
const rendererFactory = environment.rendererFactory;
const afterRenderEventManager = environment.afterRenderEventManager;

// Check no changes mode is a dev only mode used to verify that bindings have not changed
// since they were assigned. We do not want to invoke renderer factory functions in that mode
Expand All @@ -39,7 +38,6 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {

if (!checkNoChangesMode) {
rendererFactory.begin?.();
afterRenderEventManager?.begin();
}

try {
Expand All @@ -56,9 +54,6 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
// One final flush of the effects queue to catch any effects created in `ngAfterViewInit` or
// other post-order hooks.
environment.inlineEffectRunner?.flush();

// Invoke all callbacks registered via `after*Render`, if needed.
afterRenderEventManager?.end();
}
}
}
Expand Down

0 comments on commit bc3118b

Please sign in to comment.