From bb4fbc7467d77fd7df805c14ca448040daee00f8 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 26 Apr 2024 12:33:13 -0700 Subject: [PATCH] refactor(core): Ensure DOM removal happens when no app views need refresh (#55132) This reverts commit e3d5607caf1c500ea85ea1a922fce6015d04ecd0 - reapplies original changes. --- .../animations/browser/src/create_engine.ts | 22 +++---------------- .../src/render/animation_engine_next.ts | 9 +------- .../src/render/transition_animation_engine.ts | 8 +------ .../core/src/application/application_ref.ts | 19 ++++++++++++---- .../test/acceptance/renderer_factory_spec.ts | 16 ++++++++------ .../test/change_detection_scheduler_spec.ts | 4 ---- .../async/src/async_animation_renderer.ts | 8 ++----- .../animations/src/providers.ts | 2 +- .../test/animation_renderer_spec.ts | 4 ++-- 9 files changed, 34 insertions(+), 58 deletions(-) diff --git a/packages/animations/browser/src/create_engine.ts b/packages/animations/browser/src/create_engine.ts index e0a991a942494..ab67368b60ff1 100644 --- a/packages/animations/browser/src/create_engine.ts +++ b/packages/animations/browser/src/create_engine.ts @@ -6,33 +6,17 @@ * found in the LICENSE file at https://angular.io/license */ -import {ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core'; - import {NoopAnimationStyleNormalizer} from './dsl/style_normalization/animation_style_normalizer'; import {WebAnimationsStyleNormalizer} from './dsl/style_normalization/web_animations_style_normalizer'; import {NoopAnimationDriver} from './render/animation_driver'; import {AnimationEngine} from './render/animation_engine_next'; import {WebAnimationsDriver} from './render/web_animations/web_animations_driver'; -export function createEngine( - type: 'animations' | 'noop', - doc: Document, - scheduler: ChangeDetectionScheduler | null, -): AnimationEngine { +export function createEngine(type: 'animations' | 'noop', doc: Document): AnimationEngine { // TODO: find a way to make this tree shakable. if (type === 'noop') { - return new AnimationEngine( - doc, - new NoopAnimationDriver(), - new NoopAnimationStyleNormalizer(), - scheduler, - ); + return new AnimationEngine(doc, new NoopAnimationDriver(), new NoopAnimationStyleNormalizer()); } - return new AnimationEngine( - doc, - new WebAnimationsDriver(), - new WebAnimationsStyleNormalizer(), - scheduler, - ); + return new AnimationEngine(doc, new WebAnimationsDriver(), new WebAnimationsStyleNormalizer()); } diff --git a/packages/animations/browser/src/render/animation_engine_next.ts b/packages/animations/browser/src/render/animation_engine_next.ts index 6183ebd5b6d84..eb1dd5df3377a 100644 --- a/packages/animations/browser/src/render/animation_engine_next.ts +++ b/packages/animations/browser/src/render/animation_engine_next.ts @@ -6,7 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ import {AnimationMetadata, AnimationPlayer, AnimationTriggerMetadata} from '@angular/animations'; -import {ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core'; import {TriggerAst} from '../dsl/animation_ast'; import {buildAnimationAst} from '../dsl/animation_ast_builder'; @@ -33,14 +32,8 @@ export class AnimationEngine { doc: Document, private _driver: AnimationDriver, private _normalizer: AnimationStyleNormalizer, - scheduler: ChangeDetectionScheduler | null, ) { - this._transitionEngine = new TransitionAnimationEngine( - doc.body, - _driver, - _normalizer, - scheduler, - ); + this._transitionEngine = new TransitionAnimationEngine(doc.body, _driver, _normalizer); this._timelineEngine = new TimelineAnimationEngine(doc.body, _driver, _normalizer); this._transitionEngine.onRemovalComplete = (element: any, context: any) => diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 73b8ac7eb59b6..f64e5bfa3ce95 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -14,11 +14,7 @@ import { ɵPRE_STYLE as PRE_STYLE, ɵStyleDataMap, } from '@angular/animations'; -import { - ɵChangeDetectionScheduler as ChangeDetectionScheduler, - ɵNotificationSource as NotificationSource, - ɵWritable as Writable, -} from '@angular/core'; +import {ɵWritable as Writable} from '@angular/core'; import {AnimationTimelineInstruction} from '../dsl/animation_timeline_instruction'; import {AnimationTransitionFactory} from '../dsl/animation_transition_factory'; @@ -625,7 +621,6 @@ export class TransitionAnimationEngine { public bodyNode: any, public driver: AnimationDriver, private _normalizer: AnimationStyleNormalizer, - private readonly scheduler: ChangeDetectionScheduler | null, ) {} get queuedPlayers(): TransitionAnimationPlayer[] { @@ -817,7 +812,6 @@ export class TransitionAnimationEngine { removeNode(namespaceId: string, element: any, context: any): void { if (isElementNode(element)) { - this.scheduler?.notify(NotificationSource.AnimationQueuedNodeRemoval); const ns = namespaceId ? this._fetchNamespace(namespaceId) : null; if (ns) { ns.removeNode(element, context); diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts index f0995da2406aa..b3e0da6701969 100644 --- a/packages/core/src/application/application_ref.ts +++ b/packages/core/src/application/application_ref.ts @@ -27,6 +27,7 @@ import {ComponentFactoryResolver} from '../linker/component_factory_resolver'; 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 {ComponentFactory as R3ComponentFactory} from '../render3/component_ref'; import {isStandalone} from '../render3/definition'; @@ -162,6 +163,10 @@ export interface BootstrapOptions { /** Maximum number of times ApplicationRef will refresh all attached views in a single tick. */ const MAXIMUM_REFRESH_RERUNS = 10; +// This is a temporary type to represent an instance of an R3Injector, which can be destroyed. +// The type will be replaced with a different one once destroyable injector type is available. +type DestroyableInjector = Injector&{destroy?: Function, destroyed?: boolean}; + export function _callAndReportToErrorHandler( errorHandler: ErrorHandler, ngZone: NgZone, callback: () => any): any { try { @@ -546,6 +551,11 @@ export class ApplicationRef { } private detectChangesInAttachedViews(refreshViews: boolean) { + let rendererFactory: RendererFactory2|null = null; + if (!(this._injector as DestroyableInjector).destroyed) { + rendererFactory = this._injector.get(RendererFactory2, null, {optional: true}); + } + let runs = 0; const afterRenderEffectManager = this.afterRenderEffectManager; while (runs < MAXIMUM_REFRESH_RERUNS) { @@ -567,6 +577,11 @@ export class ApplicationRef { continue; } + // Flush animations before running afterRender hooks + // This might not have happened if no views were refreshed above + rendererFactory?.begin?.(); + rendererFactory?.end?.(); + afterRenderEffectManager.execute(); // If after running all afterRender callbacks we have no more views that need to be refreshed, // we can break out of the loop @@ -671,10 +686,6 @@ export class ApplicationRef { ngDevMode && 'This instance of the `ApplicationRef` has already been destroyed.'); } - // This is a temporary type to represent an instance of an R3Injector, which can be destroyed. - // The type will be replaced with a different one once destroyable injector type is available. - type DestroyableInjector = Injector&{destroy?: Function, destroyed?: boolean}; - const injector = this._injector as DestroyableInjector; // Check that this injector instance supports destroy operation. diff --git a/packages/core/test/acceptance/renderer_factory_spec.ts b/packages/core/test/acceptance/renderer_factory_spec.ts index fdb7a09c45d5e..4209182d0521d 100644 --- a/packages/core/test/acceptance/renderer_factory_spec.ts +++ b/packages/core/test/acceptance/renderer_factory_spec.ts @@ -83,20 +83,22 @@ describe('renderer factory lifecycle', () => { it('should work with a component', () => { const fixture = TestBed.createComponent(SomeComponent); - fixture.detectChanges(); - expect(logs).toEqual( - ['create', 'create', 'begin', 'some_component create', 'some_component update', 'end']); + fixture.componentRef.changeDetectorRef.detectChanges(); + expect(logs).toEqual([ + 'create', 'create', 'begin', 'end', 'begin', 'some_component create', 'some_component update', + 'end' + ]); logs = []; - fixture.detectChanges(); + fixture.componentRef.changeDetectorRef.detectChanges(); expect(logs).toEqual(['begin', 'some_component update', 'end']); }); it('should work with a component which throws', () => { expect(() => { const fixture = TestBed.createComponent(SomeComponentWhichThrows); - fixture.detectChanges(); + fixture.componentRef.changeDetectorRef.detectChanges(); }).toThrow(); - expect(logs).toEqual(['create', 'create', 'begin', 'end']); + expect(logs).toEqual(['create', 'create', 'begin', 'end', 'begin', 'end']); }); it('should pass in the component styles directly into the underlying renderer', () => { @@ -339,7 +341,7 @@ function getAnimationRendererFactory2(document: Document): RendererFactory2 { return new ɵAnimationRendererFactory( getRendererFactory2(document), new ɵAnimationEngine( - document, new MockAnimationDriver(), new ɵNoopAnimationStyleNormalizer(), null), + document, new MockAnimationDriver(), new ɵNoopAnimationStyleNormalizer()), fakeNgZone); } diff --git a/packages/core/test/change_detection_scheduler_spec.ts b/packages/core/test/change_detection_scheduler_spec.ts index ffc009dde5127..d303680400acd 100644 --- a/packages/core/test/change_detection_scheduler_spec.ts +++ b/packages/core/test/change_detection_scheduler_spec.ts @@ -333,10 +333,6 @@ describe('Angular with zoneless enabled', () => { await whenStable(); expect(host.innerHTML).toEqual('binding'); - const component2 = createComponent(DynamicCmp, {environmentInjector}); - // TODO(atscott): Only needed because renderFactory will not run if ApplicationRef has no - // views This should likely be fixed in ApplicationRef - appRef.attachView(component2.hostView); appRef.detachView(component.hostView); // DOM is not synchronously removed because change detection hasn't run expect(host.innerHTML).toEqual('binding'); 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 e0f764f05cdf4..06e8c87d4af5b 100644 --- a/packages/platform-browser/animations/async/src/async_animation_renderer.ts +++ b/packages/platform-browser/animations/async/src/async_animation_renderer.ts @@ -45,11 +45,7 @@ export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory private zone: NgZone, private animationType: 'animations' | 'noop', private moduleImpl?: Promise<{ - ɵcreateEngine: ( - type: 'animations' | 'noop', - doc: Document, - scheduler: ChangeDetectionScheduler | null, - ) => AnimationEngine; + ɵcreateEngine: (type: 'animations' | 'noop', doc: Document) => AnimationEngine; ɵAnimationRendererFactory: typeof AnimationRendererFactory; }>, ) {} @@ -84,7 +80,7 @@ export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory .then(({ɵcreateEngine, ɵAnimationRendererFactory}) => { // We can't create the renderer yet because we might need the hostElement and the type // Both are provided in createRenderer(). - this._engine = ɵcreateEngine(this.animationType, this.doc, this.scheduler); + this._engine = ɵcreateEngine(this.animationType, this.doc); const rendererFactory = new ɵAnimationRendererFactory( this.delegate, this._engine, diff --git a/packages/platform-browser/animations/src/providers.ts b/packages/platform-browser/animations/src/providers.ts index 1313efc5cbae8..0259222efb80d 100644 --- a/packages/platform-browser/animations/src/providers.ts +++ b/packages/platform-browser/animations/src/providers.ts @@ -39,7 +39,7 @@ export class InjectableAnimationEngine extends AnimationEngine implements OnDest driver: AnimationDriver, normalizer: AnimationStyleNormalizer, ) { - super(doc, driver, normalizer, inject(ChangeDetectionScheduler, {optional: true})); + super(doc, driver, normalizer); } ngOnDestroy(): void { diff --git a/packages/platform-browser/animations/test/animation_renderer_spec.ts b/packages/platform-browser/animations/test/animation_renderer_spec.ts index 6afc4cbd3a086..516617b43444b 100644 --- a/packages/platform-browser/animations/test/animation_renderer_spec.ts +++ b/packages/platform-browser/animations/test/animation_renderer_spec.ts @@ -351,11 +351,11 @@ import {el} from '../../testing/src/browser_util'; const cmp = fixture.componentInstance; renderer.log = []; - fixture.detectChanges(); + fixture.changeDetectorRef.detectChanges(); expect(renderer.log).toEqual(['begin', 'end']); renderer.log = []; - fixture.detectChanges(); + fixture.changeDetectorRef.detectChanges(); expect(renderer.log).toEqual(['begin', 'end']); }); });