Skip to content

Commit

Permalink
refactor(core): only run checkNoChanges once per tick (#54074)
Browse files Browse the repository at this point in the history
This change updates the `checkNoChanges` pass to only run once after
both view refresh and `afterRender` hooks execute rather than both
before and after the hooks. The original motivation was to specifically
ensure that the application was in a "clean state" before running the
`afterRender` hooks and ensure that `afterRender` hooks don't "fix"
`ExpressionChanged...` errors. This, however, adds to the complexity and
cost of running change detection in dev mode. Instead, the
`checkNoChanges` pass runs once we have done all render-related work and
want to ensure that the application state has been synced to the DOM
(without additional changes).

PR Close #54074
  • Loading branch information
atscott authored and thePunderWoman committed Jan 31, 2024
1 parent 67eb402 commit 2a0ac20
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 15 deletions.
9 changes: 2 additions & 7 deletions packages/core/src/application/application_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -549,13 +549,8 @@ export class ApplicationRef {
for (let view of this._views) {
view.detectChanges();
}
if (typeof ngDevMode === 'undefined' || ngDevMode) {
for (let view of this._views) {
view.checkNoChanges();
}
}
const callbacksExecuted = this.afterRenderEffectManager.execute();
if ((typeof ngDevMode === 'undefined' || ngDevMode) && callbacksExecuted) {
this.afterRenderEffectManager.execute();
if ((typeof ngDevMode === 'undefined' || ngDevMode)) {
for (let view of this._views) {
view.checkNoChanges();
}
Expand Down
12 changes: 4 additions & 8 deletions packages/core/src/render3/after_render_hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ interface AfterRenderCallbackHandler {
/**
* Execute callbacks. Returns `true` if any callbacks were executed.
*/
execute(): boolean;
execute(): void;

/**
* Perform any necessary cleanup.
Expand Down Expand Up @@ -397,12 +397,10 @@ class AfterRenderCallbackHandlerImpl implements AfterRenderCallbackHandler {
this.deferredCallbacks.delete(callback);
}

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

destroy(): void {
Expand All @@ -437,7 +434,7 @@ export class AfterRenderEventManager {
/**
* Executes callbacks. Returns `true` if any callbacks executed.
*/
execute(): boolean {
execute(): void {
// 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.
Expand All @@ -446,8 +443,7 @@ export class AfterRenderEventManager {
for (const callback of callbacks) {
callback();
}
const handlerCallbacksExecuted = this.handler?.execute();
return !!handlerCallbacksExecuted || callbacks.length > 0;
this.handler?.execute();
}

ngOnDestroy() {
Expand Down

0 comments on commit 2a0ac20

Please sign in to comment.