From 70b478ff5916cb51fa4e169aa8865e52d25ef791 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Wed, 24 Apr 2024 16:11:13 -0700 Subject: [PATCH] Revert "refactor(core): Omit listeners from out-of-zone scheduling when using ZoneJS (#55492)" (#55524) This reverts commit 617bc3337d0278233ced821c76f04c75805bbad5. PR Close #55524 --- .../scheduling/zoneless_scheduling.ts | 41 ++----------------- .../scheduling/zoneless_scheduling_impl.ts | 38 +++-------------- packages/core/src/core_private_export.ts | 2 +- packages/core/src/defer/instructions.ts | 3 +- .../core/src/render3/after_render_hooks.ts | 4 +- packages/core/src/render3/component_ref.ts | 4 +- .../core/src/render3/instructions/listener.ts | 3 +- .../render3/instructions/mark_view_dirty.ts | 6 +-- .../core/src/render3/node_manipulation.ts | 8 ++-- .../render3/util/change_detection_utils.ts | 3 +- packages/core/src/render3/util/view_utils.ts | 6 +-- packages/core/src/render3/view_ref.ts | 3 +- .../test/change_detection_scheduler_spec.ts | 41 +------------------ .../async/src/async_animation_renderer.ts | 5 ++- 14 files changed, 31 insertions(+), 136 deletions(-) diff --git a/packages/core/src/change_detection/scheduling/zoneless_scheduling.ts b/packages/core/src/change_detection/scheduling/zoneless_scheduling.ts index 8ae6835377b..78248bf4a8c 100644 --- a/packages/core/src/change_detection/scheduling/zoneless_scheduling.ts +++ b/packages/core/src/change_detection/scheduling/zoneless_scheduling.ts @@ -8,49 +8,16 @@ import {InjectionToken} from '../../di/injection_token'; - -export const enum NotificationSource { - // Change detection needs to run in order to synchronize application state - // with the DOM when the following notifications are received: - // This operation indicates that a subtree needs to be traversed during change detection. - MarkAncestorsForTraversal, - // A component/directive gets a new input. - SetInput, - // Defer block state updates need change detection to fully render the state. - DeferBlockStateUpdate, - // Debugging tools updated state and have requested change detection. - DebugApplyChanges, - // ChangeDetectorRef.markForCheck indicates the component is dirty/needs to refresh. - MarkForCheck, - - // Bound listener callbacks execute and can update state without causing other notifications from - // above. - Listener, - - // 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, - // 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 - // run render hooks). If the attached view is dirty, the `MarkAncestorsForTraversal` - // notification should also be received. - ViewAttached, - // When DOM removal happens, render hooks may be interested in the new - // DOM state but we do not need to refresh any views unless. If change - // detection is required after DOM removal, another notification should - // be received (i.e. `markForCheck`). - ViewDetachedFromDOM, - // Applying animations might result in new DOM state and should rerun render hooks - AsyncAnimationsLoaded +export const enum NotificationType { + RefreshViews, + AfterRenderHooks, } /** * Injectable that is notified when an `LView` is made aware of changes to application state. */ export abstract class ChangeDetectionScheduler { - abstract notify(source: NotificationSource): void; + abstract notify(source?: NotificationType): void; abstract runningTick: boolean; } diff --git a/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts b/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts index 61ac89b3a8a..fbb58785c66 100644 --- a/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts +++ b/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts @@ -17,7 +17,7 @@ import {scheduleCallbackWithMicrotask, scheduleCallbackWithRafRace} from '../../ import {performanceMarkFeature} from '../../util/performance'; import {NgZone, NoopNgZone} from '../../zone/ng_zone'; -import {ChangeDetectionScheduler, NotificationSource, ZONELESS_ENABLED, ZONELESS_SCHEDULER_DISABLED} from './zoneless_scheduling'; +import {ChangeDetectionScheduler, NotificationType, ZONELESS_ENABLED, ZONELESS_SCHEDULER_DISABLED} from './zoneless_scheduling'; const CONSECUTIVE_MICROTASK_NOTIFICATION_LIMIT = 100; let consecutiveMicrotaskNotifications = 0; @@ -75,38 +75,10 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler { !this.zoneIsDefined); } - notify(source: NotificationSource): void { - if (!this.zonelessEnabled && source === NotificationSource.Listener) { - // When the notification comes from a listener, we skip the notification unless the - // application has enabled zoneless. Ideally, listeners wouldn't notify the scheduler at all - // automatically. We do not know that a developer made a change in the listener callback that - // requires an `ApplicationRef.tick` (synchronize templates / run render hooks). We do this - // only for an easier migration from OnPush components to zoneless. Because listeners are - // usually executed inside the Angular zone and listeners automatically call `markViewDirty`, - // developers never needed to manually use `ChangeDetectorRef.markForCheck` or some other API - // to make listener callbacks work correctly with `OnPush` components. - return; - } - switch (source) { - case NotificationSource.DebugApplyChanges: - case NotificationSource.DeferBlockStateUpdate: - case NotificationSource.MarkAncestorsForTraversal: - case NotificationSource.MarkForCheck: - case NotificationSource.Listener: - case NotificationSource.SetInput: { - this.shouldRefreshViews = true; - break; - } - case NotificationSource.ViewDetachedFromDOM: - case NotificationSource.ViewAttached: - case NotificationSource.NewRenderHook: - case NotificationSource.AsyncAnimationsLoaded: - default: { - // These notifications only schedule a tick but do not change whether we should refresh - // views. Instead, we only need to run render hooks unless another notification from the - // other set is also received before `tick` happens. - } - } + notify(type = NotificationType.RefreshViews): void { + // When the only source of notification is an afterRender hook will skip straight to the hooks + // rather than refreshing views in ApplicationRef.tick + this.shouldRefreshViews ||= type === NotificationType.RefreshViews; if (!this.shouldScheduleTick()) { return; diff --git a/packages/core/src/core_private_export.ts b/packages/core/src/core_private_export.ts index 18a6750c546..1b865bf468b 100644 --- a/packages/core/src/core_private_export.ts +++ b/packages/core/src/core_private_export.ts @@ -11,7 +11,7 @@ export {detectChangesInViewIfRequired as ɵdetectChangesInViewIfRequired, whenSt export {IMAGE_CONFIG as ɵIMAGE_CONFIG, IMAGE_CONFIG_DEFAULTS as ɵIMAGE_CONFIG_DEFAULTS, ImageConfig as ɵImageConfig} from './application/application_tokens'; export {internalCreateApplication as ɵinternalCreateApplication} from './application/create_application'; export {defaultIterableDiffers as ɵdefaultIterableDiffers, defaultKeyValueDiffers as ɵdefaultKeyValueDiffers} from './change_detection/change_detection'; -export {ChangeDetectionScheduler as ɵChangeDetectionScheduler, NotificationSource as ɵNotificationSource, ZONELESS_ENABLED as ɵZONELESS_ENABLED} from './change_detection/scheduling/zoneless_scheduling'; +export {ChangeDetectionScheduler as ɵChangeDetectionScheduler, NotificationType as ɵNotificationType, ZONELESS_ENABLED as ɵZONELESS_ENABLED} from './change_detection/scheduling/zoneless_scheduling'; export {Console as ɵConsole} from './console'; export {DeferBlockDetails as ɵDeferBlockDetails, getDeferBlocks as ɵgetDeferBlocks} from './defer/discovery'; export {renderDeferBlockState as ɵrenderDeferBlockState, triggerResourceLoading as ɵtriggerResourceLoading} from './defer/instructions'; diff --git a/packages/core/src/defer/instructions.ts b/packages/core/src/defer/instructions.ts index c8e7c03803a..2ace644f197 100644 --- a/packages/core/src/defer/instructions.ts +++ b/packages/core/src/defer/instructions.ts @@ -9,7 +9,6 @@ import {setActiveConsumer} from '@angular/core/primitives/signals'; import {CachedInjectorService} from '../cached_injector_service'; -import {NotificationSource} from '../change_detection/scheduling/zoneless_scheduling'; import {EnvironmentInjector, InjectionToken, Injector} from '../di'; import {internalImportProvidersFrom} from '../di/provider_collection'; import {RuntimeError, RuntimeErrorCode} from '../errors'; @@ -591,7 +590,7 @@ function applyDeferBlockState( createAndRenderEmbeddedLView(hostLView, activeBlockTNode, null, {dehydratedView, injector}); addLViewToLContainer( lContainer, embeddedLView, viewIndex, shouldAddViewToDom(activeBlockTNode, dehydratedView)); - markViewDirty(embeddedLView, NotificationSource.DeferBlockStateUpdate); + markViewDirty(embeddedLView); } } diff --git a/packages/core/src/render3/after_render_hooks.ts b/packages/core/src/render3/after_render_hooks.ts index d4b85fee159..a03d945c44a 100644 --- a/packages/core/src/render3/after_render_hooks.ts +++ b/packages/core/src/render3/after_render_hooks.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ChangeDetectionScheduler, NotificationSource} from '../change_detection/scheduling/zoneless_scheduling'; +import {ChangeDetectionScheduler, NotificationType} from '../change_detection/scheduling/zoneless_scheduling'; import {assertInInjectionContext, Injector, runInInjectionContext, ɵɵdefineInjectable} from '../di'; import {inject} from '../di/injector_compatibility'; import {ErrorHandler} from '../error_handler'; @@ -336,7 +336,7 @@ class AfterRenderCallback { constructor(readonly phase: AfterRenderPhase, private callbackFn: VoidFunction) { // Registering a callback will notify the scheduler. - inject(ChangeDetectionScheduler, {optional: true})?.notify(NotificationSource.NewRenderHook); + inject(ChangeDetectionScheduler, {optional: true})?.notify(NotificationType.AfterRenderHooks); } invoke() { diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index 933cc8c0c02..80de0a70633 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -9,7 +9,7 @@ import {setActiveConsumer} from '@angular/core/primitives/signals'; import {ChangeDetectorRef} from '../change_detection/change_detector_ref'; -import {ChangeDetectionScheduler, NotificationSource} from '../change_detection/scheduling/zoneless_scheduling'; +import {ChangeDetectionScheduler} from '../change_detection/scheduling/zoneless_scheduling'; import {Injector} from '../di/injector'; import {convertToBitFlags} from '../di/injector_compatibility'; import {InjectFlags, InjectOptions} from '../di/interface/injector'; @@ -365,7 +365,7 @@ export class ComponentRef extends AbstractComponentRef { setInputsForProperty(lView[TVIEW], lView, dataValue, name, value); this.previousInputValues.set(name, value); const childComponentLView = getComponentLViewByIndex(this._tNode.index, lView); - markViewDirty(childComponentLView, NotificationSource.SetInput); + markViewDirty(childComponentLView); } else { if (ngDevMode) { const cmpNameForError = stringifyForError(this.componentType); diff --git a/packages/core/src/render3/instructions/listener.ts b/packages/core/src/render3/instructions/listener.ts index d64f6a88e4c..cc692000ab2 100644 --- a/packages/core/src/render3/instructions/listener.ts +++ b/packages/core/src/render3/instructions/listener.ts @@ -9,7 +9,6 @@ import {setActiveConsumer} from '@angular/core/primitives/signals'; -import {NotificationSource} from '../../change_detection/scheduling/zoneless_scheduling'; import {assertIndexInRange} from '../../util/assert'; import {NodeOutputBindings, TNode, TNodeType} from '../interfaces/node'; import {GlobalTargetResolver, Renderer} from '../interfaces/renderer'; @@ -261,7 +260,7 @@ function wrapListener( // must also mark the component view itself dirty (i.e. the view that it owns). const startView = tNode.componentOffset > -1 ? getComponentLViewByIndex(tNode.index, lView) : lView; - markViewDirty(startView, NotificationSource.Listener); + markViewDirty(startView); let result = executeListenerWithErrorHandling(lView, context, listenerFn, e); // A just-invoked listener function might have coalesced listeners so we need to check for diff --git a/packages/core/src/render3/instructions/mark_view_dirty.ts b/packages/core/src/render3/instructions/mark_view_dirty.ts index 380e0696f42..0f8d276ffed 100644 --- a/packages/core/src/render3/instructions/mark_view_dirty.ts +++ b/packages/core/src/render3/instructions/mark_view_dirty.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {NotificationSource} from '../../change_detection/scheduling/zoneless_scheduling'; +import {NotificationType} from '../../change_detection/scheduling/zoneless_scheduling'; import {isRootView} from '../interfaces/type_checks'; import {ENVIRONMENT, FLAGS, LView, LViewFlags} from '../interfaces/view'; import {isRefreshingViews} from '../state'; @@ -23,7 +23,7 @@ import {getLViewParent} from '../util/view_utils'; * @param lView The starting LView to mark dirty * @returns the root LView */ -export function markViewDirty(lView: LView, source: NotificationSource): LView|null { +export function markViewDirty(lView: LView): LView|null { const dirtyBitsToUse = isRefreshingViews() ? // When we are actively refreshing views, we only use the `Dirty` bit to mark a view // for check. This bit is ignored in ChangeDetectionMode.Targeted, which is used to @@ -37,7 +37,7 @@ export function markViewDirty(lView: LView, source: NotificationSource): LView|n // afterRender hooks as well as animation listeners which execute after detecting // changes in a view when the render factory flushes. LViewFlags.RefreshView | LViewFlags.Dirty; - lView[ENVIRONMENT].changeDetectionScheduler?.notify(source); + lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationType.RefreshViews); while (lView) { lView[FLAGS] |= dirtyBitsToUse; const parent = getLViewParent(lView); diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 0ff1c8b1eaa..527f30642e4 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -8,7 +8,7 @@ import {consumerDestroy, getActiveConsumer, setActiveConsumer} from '@angular/core/primitives/signals'; -import {NotificationSource} from '../change_detection/scheduling/zoneless_scheduling'; +import {NotificationType} from '../change_detection/scheduling/zoneless_scheduling'; import {hasInSkipHydrationBlockFlag} from '../hydration/skip_hydration'; import {ViewEncapsulation} from '../metadata/view'; import {RendererStyleFlags2} from '../render/api_flags'; @@ -174,10 +174,8 @@ export function addViewToDOM( * @param lView the `LView` to be detached. */ export function detachViewFromDOM(tView: TView, lView: LView) { - // When we remove a view from the DOM, we need to rerun afterRender hooks - // We don't necessarily needs to run change detection. DOM removal only requires - // change detection if animations are enabled (this notification is handled by animations). - lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationSource.ViewDetachedFromDOM); + // When we remove a view from the DOM, we need to rerun afterRender hooks. + lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationType.AfterRenderHooks); applyView(tView, lView, lView[RENDERER], WalkTNodeTreeAction.Detach, null, null); } diff --git a/packages/core/src/render3/util/change_detection_utils.ts b/packages/core/src/render3/util/change_detection_utils.ts index 03159cc07e5..bd20dcdad4c 100644 --- a/packages/core/src/render3/util/change_detection_utils.ts +++ b/packages/core/src/render3/util/change_detection_utils.ts @@ -6,7 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -import {NotificationSource} from '../../change_detection/scheduling/zoneless_scheduling'; import {assertDefined} from '../../util/assert'; import {getComponentViewByInstance} from '../context_discovery'; import {detectChangesInternal} from '../instructions/change_detection'; @@ -26,7 +25,7 @@ import {getRootComponents} from './discovery_utils'; */ export function applyChanges(component: {}): void { ngDevMode && assertDefined(component, 'component'); - markViewDirty(getComponentViewByInstance(component), NotificationSource.DebugApplyChanges); + markViewDirty(getComponentViewByInstance(component)); getRootComponents(component).forEach(rootComponent => detectChanges(rootComponent)); } diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index 817c6271b81..37777fe9d39 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {NotificationSource} from '../../change_detection/scheduling/zoneless_scheduling'; +import {NotificationType} from '../../change_detection/scheduling/zoneless_scheduling'; import {RuntimeError, RuntimeErrorCode} from '../../errors'; import {assertDefined, assertGreaterThan, assertGreaterThanOrEqual, assertIndexInRange, assertLessThan} from '../../util/assert'; import {assertLView, assertTNode, assertTNodeForLView} from '../assert'; @@ -208,7 +208,7 @@ export function requiresRefreshOrTraversal(lView: LView) { * parents above. */ export function updateAncestorTraversalFlagsOnAttach(lView: LView) { - lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationSource.ViewAttached); + lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationType.AfterRenderHooks); if (lView[FLAGS] & LViewFlags.Dirty) { lView[FLAGS] |= LViewFlags.RefreshView; } @@ -225,7 +225,7 @@ export function updateAncestorTraversalFlagsOnAttach(lView: LView) { * flag is already `true` or the `lView` is detached. */ export function markAncestorsForTraversal(lView: LView) { - lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationSource.MarkAncestorsForTraversal); + lView[ENVIRONMENT].changeDetectionScheduler?.notify(NotificationType.RefreshViews); let parent = getLViewParent(lView); while (parent !== null) { // We stop adding markers to the ancestors once we reach one that already has the marker. This diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 2e350bbdbed..2a4cfaaf902 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -7,7 +7,6 @@ */ import {ChangeDetectorRef} from '../change_detection/change_detector_ref'; -import {NotificationSource} from '../change_detection/scheduling/zoneless_scheduling'; import {RuntimeError, RuntimeErrorCode} from '../errors'; import {EmbeddedViewRef, ViewRefTracker} from '../linker/view_ref'; import {removeFromArray} from '../util/array_utils'; @@ -142,7 +141,7 @@ export class ViewRef implements EmbeddedViewRef, ChangeDetectorRefInterfac * ``` */ markForCheck(): void { - markViewDirty(this._cdRefInjectingView || this._lView, NotificationSource.MarkForCheck); + markViewDirty(this._cdRefInjectingView || this._lView); } /** diff --git a/packages/core/test/change_detection_scheduler_spec.ts b/packages/core/test/change_detection_scheduler_spec.ts index 3bddfe416ee..21beedca77c 100644 --- a/packages/core/test/change_detection_scheduler_spec.ts +++ b/packages/core/test/change_detection_scheduler_spec.ts @@ -8,7 +8,7 @@ import {AsyncPipe} from '@angular/common'; import {PLATFORM_BROWSER_ID} from '@angular/common/src/platform_id'; -import {afterNextRender, afterRender, ApplicationRef, ChangeDetectorRef, Component, createComponent, destroyPlatform, Directive, ElementRef, EnvironmentInjector, ErrorHandler, EventEmitter, inject, Input, NgZone, Output, PLATFORM_ID, provideExperimentalZonelessChangeDetection as provideZonelessChangeDetection, provideZoneChangeDetection, signal, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; +import {afterNextRender, afterRender, ApplicationRef, ChangeDetectorRef, Component, createComponent, destroyPlatform, ElementRef, EnvironmentInjector, ErrorHandler, inject, Input, NgZone, PLATFORM_ID, provideExperimentalZonelessChangeDetection as provideZonelessChangeDetection, provideZoneChangeDetection, signal, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; import {toSignal} from '@angular/core/rxjs-interop'; import {ComponentFixture, ComponentFixtureAutoDetect, TestBed} from '@angular/core/testing'; import {bootstrapApplication} from '@angular/platform-browser'; @@ -650,45 +650,6 @@ describe('Angular with scheduler and ZoneJS', () => { expect(fixture.nativeElement.innerText).toContain('initial'); }); - it('will not schedule change detection if listener callback is outside the zone', async () => { - let renders = 0; - TestBed.runInInjectionContext(() => { - afterRender(() => { - renders++; - }); - }); - - @Component({selector: 'component-with-output', template: '', standalone: true}) - class ComponentWithOutput { - @Output() out = new EventEmitter(); - } - let called = false; - @Component({ - standalone: true, - imports: [ComponentWithOutput], - template: '' - }) - class App { - onOut() { - called = true; - } - } - const fixture = TestBed.createComponent(App); - await fixture.whenStable(); - const outComponent = - fixture.debugElement - .query((debugNode) => debugNode.providerTokens!.indexOf(ComponentWithOutput) !== -1) - .componentInstance as ComponentWithOutput; - TestBed.inject(NgZone).runOutsideAngular(() => { - outComponent.out.emit(); - }); - await fixture.whenStable(); - - expect(renders).toBe(1); - expect(called).toBe(true); - expect(renders).toBe(1); - }); - it('updating signal outside of zone still schedules update when in hybrid mode', async () => { @Component({template: '{{thing()}}', standalone: true}) class App { diff --git a/packages/platform-browser/animations/async/src/async_animation_renderer.ts b/packages/platform-browser/animations/async/src/async_animation_renderer.ts index 06e8c87d4af..fac2708736a 100644 --- a/packages/platform-browser/animations/async/src/async_animation_renderer.ts +++ b/packages/platform-browser/animations/async/src/async_animation_renderer.ts @@ -22,7 +22,7 @@ import { RendererType2, ɵAnimationRendererType as AnimationRendererType, ɵChangeDetectionScheduler as ChangeDetectionScheduler, - ɵNotificationSource as NotificationSource, + ɵNotificationType as NotificationType, ɵRuntimeError as RuntimeError, } from '@angular/core'; import {ɵRuntimeErrorCode as RuntimeErrorCode} from '@angular/platform-browser'; @@ -128,7 +128,8 @@ export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory rendererType, ); dynamicRenderer.use(animationRenderer); - this.scheduler?.notify(NotificationSource.AsyncAnimationsLoaded); + // Applying animations might result in new DOM state and should rerun render hooks + this.scheduler?.notify(NotificationType.AfterRenderHooks); }) .catch((e) => { // Permanently use regular renderer when loading fails.