Skip to content

Commit 7620f50

Browse files
atscottpkozlowski-opensource
atscott
authored and
pkozlowski-opensource
committed
refactor(core): node removal notifies scheduler only when animations are enabled (#53857)
Node removal is immediate and does not require change detection to run when animations are not provided. This refactor makes the animation engine notify the scheduler rather than doing it on all node removals. PR Close #53857
1 parent 62ad2f0 commit 7620f50

10 files changed

Lines changed: 78 additions & 28 deletions

File tree

‎packages/animations/browser/src/create_engine.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,23 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
import {ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core';
10+
911
import {NoopAnimationStyleNormalizer} from './dsl/style_normalization/animation_style_normalizer';
1012
import {WebAnimationsStyleNormalizer} from './dsl/style_normalization/web_animations_style_normalizer';
1113
import {NoopAnimationDriver} from './render/animation_driver';
1214
import {AnimationEngine} from './render/animation_engine_next';
1315
import {WebAnimationsDriver} from './render/web_animations/web_animations_driver';
1416

15-
export function createEngine(type: 'animations'|'noop', doc: Document): AnimationEngine {
17+
export function createEngine(
18+
type: 'animations'|'noop', doc: Document,
19+
scheduler: ChangeDetectionScheduler|null): AnimationEngine {
1620
// TODO: find a way to make this tree shakable.
1721
if (type === 'noop') {
18-
return new AnimationEngine(doc, new NoopAnimationDriver(), new NoopAnimationStyleNormalizer());
22+
return new AnimationEngine(
23+
doc, new NoopAnimationDriver(), new NoopAnimationStyleNormalizer(), scheduler);
1924
}
2025

21-
return new AnimationEngine(doc, new WebAnimationsDriver(), new WebAnimationsStyleNormalizer());
26+
return new AnimationEngine(
27+
doc, new WebAnimationsDriver(), new WebAnimationsStyleNormalizer(), scheduler);
2228
}

‎packages/animations/browser/src/render/animation_engine_next.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88
import {AnimationMetadata, AnimationPlayer, AnimationTriggerMetadata} from '@angular/animations';
9+
import {ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core';
910

1011
import {TriggerAst} from '../dsl/animation_ast';
1112
import {buildAnimationAst} from '../dsl/animation_ast_builder';
@@ -30,8 +31,9 @@ export class AnimationEngine {
3031

3132
constructor(
3233
doc: Document, private _driver: AnimationDriver,
33-
private _normalizer: AnimationStyleNormalizer) {
34-
this._transitionEngine = new TransitionAnimationEngine(doc.body, _driver, _normalizer);
34+
private _normalizer: AnimationStyleNormalizer, scheduler: ChangeDetectionScheduler|null) {
35+
this._transitionEngine =
36+
new TransitionAnimationEngine(doc.body, _driver, _normalizer, scheduler);
3537
this._timelineEngine = new TimelineAnimationEngine(doc.body, _driver, _normalizer);
3638

3739
this._transitionEngine.onRemovalComplete = (element: any, context: any) =>

‎packages/animations/browser/src/render/transition_animation_engine.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88
import {AnimationOptions, AnimationPlayer, AUTO_STYLE, NoopAnimationPlayer, ɵAnimationGroupPlayer as AnimationGroupPlayer, ɵPRE_STYLE as PRE_STYLE, ɵStyleDataMap} from '@angular/animations';
9-
import {ɵWritable as Writable} from '@angular/core';
9+
import {ɵChangeDetectionScheduler as ChangeDetectionScheduler, ɵWritable as Writable} from '@angular/core';
1010

1111
import {AnimationTimelineInstruction} from '../dsl/animation_timeline_instruction';
1212
import {AnimationTransitionFactory} from '../dsl/animation_transition_factory';
@@ -547,7 +547,8 @@ export class TransitionAnimationEngine {
547547

548548
constructor(
549549
public bodyNode: any, public driver: AnimationDriver,
550-
private _normalizer: AnimationStyleNormalizer) {}
550+
private _normalizer: AnimationStyleNormalizer,
551+
private readonly scheduler: ChangeDetectionScheduler|null) {}
551552

552553
get queuedPlayers(): TransitionAnimationPlayer[] {
553554
const players: TransitionAnimationPlayer[] = [];
@@ -738,6 +739,7 @@ export class TransitionAnimationEngine {
738739

739740
removeNode(namespaceId: string, element: any, context: any): void {
740741
if (isElementNode(element)) {
742+
this.scheduler?.notify();
741743
const ns = namespaceId ? this._fetchNamespace(namespaceId) : null;
742744
if (ns) {
743745
ns.removeNode(element, context);

‎packages/animations/browser/test/render/transition_animation_engine_spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ describe('TransitionAnimationEngine', () => {
3838

3939
function makeEngine(normalizer?: AnimationStyleNormalizer) {
4040
const engine = new TransitionAnimationEngine(
41-
getBodyNode(), driver, normalizer || new NoopAnimationStyleNormalizer());
41+
getBodyNode(), driver, normalizer || new NoopAnimationStyleNormalizer(), null);
4242
engine.createNamespace(DEFAULT_NAMESPACE_ID, element);
4343
return engine;
4444
}

‎packages/core/src/render3/node_manipulation.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,6 @@ export function addViewToDOM(
173173
* @param lView the `LView` to be detached.
174174
*/
175175
export function detachViewFromDOM(tView: TView, lView: LView) {
176-
// The scheduler must be notified because the animation engine is what actually does the DOM
177-
// removal and only runs at the end of change detection.
178-
lView[ENVIRONMENT].changeDetectionScheduler?.notify();
179176
applyView(tView, lView, lView[RENDERER], WalkTNodeTreeAction.Detach, null, null);
180177
}
181178

‎packages/core/test/acceptance/renderer_factory_spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ function getAnimationRendererFactory2(document: Document): RendererFactory2 {
339339
return new ɵAnimationRendererFactory(
340340
getRendererFactory2(document),
341341
new ɵAnimationEngine(
342-
document, new MockAnimationDriver(), new ɵNoopAnimationStyleNormalizer()),
342+
document, new MockAnimationDriver(), new ɵNoopAnimationStyleNormalizer(), null),
343343
fakeNgZone);
344344
}
345345

‎packages/core/test/change_detection_scheduler_spec.ts‎

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,21 @@
77
*/
88

99
import {AsyncPipe} from '@angular/common';
10-
import {ApplicationRef, ChangeDetectorRef, Component, ComponentRef, createComponent, DebugElement, ElementRef, EnvironmentInjector, ErrorHandler, getDebugNode, inject, Injectable, Input, NgZone, PLATFORM_ID, signal, TemplateRef, Type, ViewChild, ViewContainerRef, ɵprovideZonelessChangeDetection as provideZonelessChangeDetection} from '@angular/core';
10+
import {PLATFORM_BROWSER_ID} from '@angular/common/src/platform_id';
11+
import {ApplicationRef, ChangeDetectorRef, Component, ComponentRef, createComponent, DebugElement, destroyPlatform, ElementRef, EnvironmentInjector, ErrorHandler, getDebugNode, inject, Injectable, Input, NgZone, PLATFORM_ID, signal, TemplateRef, Type, ViewChild, ViewContainerRef, ɵprovideZonelessChangeDetection as provideZonelessChangeDetection} from '@angular/core';
1112
import {toSignal} from '@angular/core/rxjs-interop';
1213
import {TestBed} from '@angular/core/testing';
14+
import {bootstrapApplication} from '@angular/platform-browser';
15+
import {withBody} from '@angular/private/testing';
1316
import {BehaviorSubject, firstValueFrom} from 'rxjs';
1417
import {filter, take, tap} from 'rxjs/operators';
1518

1619
describe('Angular with NoopNgZone', () => {
17-
function whenStable(): Promise<boolean> {
18-
return firstValueFrom(TestBed.inject(EnvironmentInjector)
19-
.get(ApplicationRef)
20-
.isStable.pipe(filter(stable => stable)));
20+
function whenStable(applicationRef = TestBed.inject(ApplicationRef)): Promise<boolean> {
21+
return firstValueFrom(applicationRef.isStable.pipe(filter(stable => stable)));
2122
}
2223

23-
function isStable(): boolean {
24-
const injector = TestBed.inject(EnvironmentInjector);
24+
function isStable(injector = TestBed.inject(EnvironmentInjector)): boolean {
2525
return toSignal(injector.get(ApplicationRef).isStable, {requireSync: true, injector})();
2626
}
2727

@@ -216,7 +216,7 @@ describe('Angular with NoopNgZone', () => {
216216
expect(componentRef.location.nativeElement.innerText).toEqual('binding');
217217
});
218218

219-
it('when destroying a view', async () => {
219+
it('when destroying a view (with animations)', async () => {
220220
@Component({
221221
template: '{{"binding"}}',
222222
standalone: true,
@@ -249,10 +249,48 @@ describe('Angular with NoopNgZone', () => {
249249
await whenStable();
250250
expect(fixture.location.nativeElement.innerText).toEqual('binding');
251251
component2.destroy();
252+
expect(isStable()).toBe(false);
252253
await whenStable();
253254
expect(fixture.location.nativeElement.innerText).toEqual('');
254255
});
255256

257+
it('when destroying a view (*no* animations)', withBody('<app></app>', async () => {
258+
destroyPlatform();
259+
@Component({
260+
template: '{{"binding"}}',
261+
standalone: true,
262+
})
263+
class DynamicCmp {
264+
elementRef = inject(ElementRef);
265+
}
266+
@Component({
267+
selector: 'app',
268+
template: '<ng-template #ref></ng-template>',
269+
standalone: true,
270+
})
271+
class App {
272+
@ViewChild('ref', {read: ViewContainerRef}) viewContainer!: ViewContainerRef;
273+
}
274+
const applicationRef = await bootstrapApplication(App, {
275+
providers: [
276+
provideZonelessChangeDetection(),
277+
{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID},
278+
]
279+
});
280+
const appViewRef = (applicationRef as any)._views[0] as {context: App, rootNodes: any[]};
281+
await whenStable(applicationRef);
282+
283+
const component2 =
284+
createComponent(DynamicCmp, {environmentInjector: applicationRef.injector});
285+
appViewRef.context.viewContainer.insert(component2.hostView);
286+
expect(isStable(applicationRef.injector)).toBe(false);
287+
await whenStable(applicationRef);
288+
component2.destroy();
289+
expect(isStable(applicationRef.injector)).toBe(true);
290+
expect(appViewRef.rootNodes[0].innerText).toEqual('');
291+
destroyPlatform();
292+
}));
293+
256294
it('when attaching view to ApplicationRef', async () => {
257295
@Component({
258296
selector: 'dynamic-cmp',

‎packages/platform-browser/animations/async/src/async_animation_renderer.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@
77
*/
88

99
import {ɵAnimationEngine as AnimationEngine, ɵAnimationRenderer as AnimationRenderer, ɵAnimationRendererFactory as AnimationRendererFactory} from '@angular/animations/browser';
10-
import {NgZone, Renderer2, RendererFactory2, RendererStyleFlags2, RendererType2, ɵAnimationRendererType as AnimationRendererType, ɵRuntimeError as RuntimeError} from '@angular/core';
10+
import {inject, NgZone, Renderer2, RendererFactory2, RendererStyleFlags2, RendererType2, ɵAnimationRendererType as AnimationRendererType, ɵChangeDetectionScheduler as ChangeDetectionScheduler, ɵRuntimeError as RuntimeError} from '@angular/core';
1111
import {ɵRuntimeErrorCode as RuntimeErrorCode} from '@angular/platform-browser';
1212

1313
const ANIMATION_PREFIX = '@';
1414

1515
export class AsyncAnimationRendererFactory implements RendererFactory2 {
1616
private _rendererFactoryPromise: Promise<AnimationRendererFactory>|null = null;
17+
private readonly scheduler = inject(ChangeDetectionScheduler, {optional: true});
1718

1819
/**
1920
*
@@ -22,7 +23,9 @@ export class AsyncAnimationRendererFactory implements RendererFactory2 {
2223
constructor(
2324
private doc: Document, private delegate: RendererFactory2, private zone: NgZone,
2425
private animationType: 'animations'|'noop', private moduleImpl?: Promise<{
25-
ɵcreateEngine: (type: 'animations'|'noop', doc: Document) => AnimationEngine,
26+
ɵcreateEngine:
27+
(type: 'animations'|'noop', doc: Document,
28+
scheduler: ChangeDetectionScheduler|null) => AnimationEngine,
2629
ɵAnimationRendererFactory: typeof AnimationRendererFactory
2730
}>) {}
2831

@@ -44,7 +47,7 @@ export class AsyncAnimationRendererFactory implements RendererFactory2 {
4447
.then(({ɵcreateEngine, ɵAnimationRendererFactory}) => {
4548
// We can't create the renderer yet because we might need the hostElement and the type
4649
// Both are provided in createRenderer().
47-
const engine = ɵcreateEngine(this.animationType, this.doc);
50+
const engine = ɵcreateEngine(this.animationType, this.doc, this.scheduler);
4851
const rendererFactory = new ɵAnimationRendererFactory(this.delegate, engine, this.zone);
4952
this.delegate = rendererFactory;
5053
return rendererFactory;

‎packages/platform-browser/animations/async/test/animation_renderer_spec.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import {animate, AnimationPlayer, AnimationTriggerMetadata, style, transition, trigger} from '@angular/animations';
99
import {ɵAnimationEngine as AnimationEngine, ɵAnimationRenderer as AnimationRenderer, ɵAnimationRendererFactory as AnimationRendererFactory, ɵBaseAnimationRenderer as BaseAnimationRenderer} from '@angular/animations/browser';
1010
import {DOCUMENT} from '@angular/common';
11-
import {ANIMATION_MODULE_TYPE, Component, Injectable, NgZone, Renderer2, RendererFactory2, RendererType2, ViewChild} from '@angular/core';
11+
import {ANIMATION_MODULE_TYPE, Component, Injectable, NgZone, RendererFactory2, RendererType2, ViewChild, ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core';
1212
import {TestBed} from '@angular/core/testing';
1313
import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser';
1414
import {InjectableAnimationEngine} from '@angular/platform-browser/animations/src/providers';
@@ -37,7 +37,9 @@ describe('AnimationRenderer', () => {
3737
(doc: Document, renderer: DomRendererFactory2, zone: NgZone,
3838
engine: MockAnimationEngine) => {
3939
const animationModule = {
40-
ɵcreateEngine: (_: 'animations'|'noop', _2: Document): AnimationEngine => engine,
40+
ɵcreateEngine:
41+
(_: 'animations'|'noop', _2: Document, _3: ChangeDetectionScheduler|null):
42+
AnimationEngine => engine,
4143
ɵAnimationEngine: MockAnimationEngine as any,
4244
ɵAnimationRenderer: AnimationRenderer,
4345
ɵBaseAnimationRenderer: BaseAnimationRenderer,

‎packages/platform-browser/animations/src/providers.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {AnimationDriver, NoopAnimationDriver, ɵAnimationEngine as AnimationEngine, ɵAnimationRendererFactory as AnimationRendererFactory, ɵAnimationStyleNormalizer as AnimationStyleNormalizer, ɵWebAnimationsDriver as WebAnimationsDriver, ɵWebAnimationsStyleNormalizer as WebAnimationsStyleNormalizer} from '@angular/animations/browser';
1010
import {DOCUMENT} from '@angular/common';
11-
import {ANIMATION_MODULE_TYPE, ApplicationRef, Inject, Injectable, NgZone, OnDestroy, Provider, RendererFactory2} from '@angular/core';
11+
import {ANIMATION_MODULE_TYPE, inject, Inject, Injectable, NgZone, OnDestroy, Provider, RendererFactory2, ɵChangeDetectionScheduler as ChangeDetectionScheduler} from '@angular/core';
1212
import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser';
1313

1414
@Injectable()
@@ -18,8 +18,8 @@ export class InjectableAnimationEngine extends AnimationEngine implements OnDest
1818
// both have `ngOnDestroy` hooks and `flush()` must be called after all views are destroyed.
1919
constructor(
2020
@Inject(DOCUMENT) doc: Document, driver: AnimationDriver,
21-
normalizer: AnimationStyleNormalizer, appRef: ApplicationRef) {
22-
super(doc, driver, normalizer);
21+
normalizer: AnimationStyleNormalizer) {
22+
super(doc, driver, normalizer, inject(ChangeDetectionScheduler, {optional: true}));
2323
}
2424

2525
ngOnDestroy(): void {

0 commit comments

Comments
 (0)