Skip to content

Commit

Permalink
fix(animations): cleanup DOM elements when the root view is removed (#…
Browse files Browse the repository at this point in the history
…45143)

Currently, when importing `BrowserAnimationsModule`, Angular provides
`AnimationRendererFactory` as the `RendererFactory2`. The `AnimationRendererFactory`
relies on the `AnimationEngine`. The `AnimationEngine` may be created earlier than the
`ApplicationRef` (e.g. if it's requested within the `APP_INITIALIZER` before the `ApplicationRef`
is created). This means that Angular will add the `AnimationEngine` to `R3Injector.onDestroy`
before the `ApplicationRef`. The `R3Injector` will call `ngOnDestroy()` on the `AnimationEngine`
before the `ApplicationRef`, which means the `flush()` will be called earlier before views are destroyed.

PR Close #45108

PR Close #45143
  • Loading branch information
arturovt authored and dylhunn committed Jun 23, 2022
1 parent 187e6c3 commit 51be9bb
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 7 deletions.
9 changes: 6 additions & 3 deletions packages/core/src/application_ref.ts
Expand Up @@ -732,8 +732,10 @@ export class ApplicationRef {

/** @internal */
constructor(
private _zone: NgZone, private _injector: Injector, private _exceptionHandler: ErrorHandler,
private _initStatus: ApplicationInitStatus) {
private _zone: NgZone,
private _injector: Injector,
private _exceptionHandler: ErrorHandler,
) {
this._onMicrotaskEmptySubscription = this._zone.onMicrotaskEmpty.subscribe({
next: () => {
this._zone.run(() => {
Expand Down Expand Up @@ -914,8 +916,9 @@ export class ApplicationRef {
ComponentRef<C> {
NG_DEV_MODE && this.warnIfDestroyed();
const isComponentFactory = componentOrFactory instanceof ComponentFactory;
const initStatus = this._injector.get(ApplicationInitStatus);

if (!this._initStatus.done) {
if (!initStatus.done) {
const standalone = !isComponentFactory && isStandalone(componentOrFactory);
const errorMessage =
'Cannot bootstrap as there are still asynchronous initializers running.' +
Expand Down
8 changes: 6 additions & 2 deletions packages/platform-browser/animations/src/providers.ts
Expand Up @@ -9,16 +9,20 @@
import {AnimationBuilder} from '@angular/animations';
import {AnimationDriver, ɵAnimationEngine as AnimationEngine, ɵAnimationStyleNormalizer as AnimationStyleNormalizer, ɵNoopAnimationDriver as NoopAnimationDriver, ɵWebAnimationsDriver as WebAnimationsDriver, ɵWebAnimationsStyleNormalizer as WebAnimationsStyleNormalizer} from '@angular/animations/browser';
import {DOCUMENT} from '@angular/common';
import {ANIMATION_MODULE_TYPE, Inject, Injectable, InjectionToken, NgZone, OnDestroy, Provider, RendererFactory2} from '@angular/core';
import {ANIMATION_MODULE_TYPE, ApplicationRef, Inject, Injectable, NgZone, OnDestroy, Provider, RendererFactory2} from '@angular/core';
import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser';

import {BrowserAnimationBuilder} from './animation_builder';
import {AnimationRendererFactory} from './animation_renderer';

@Injectable()
export class InjectableAnimationEngine extends AnimationEngine implements OnDestroy {
// The `ApplicationRef` is injected here explicitly to force the dependency ordering.
// Since the `ApplicationRef` should be created earlier before the `AnimationEngine`, they
// both have `ngOnDestroy` hooks and `flush()` must be called after all views are destroyed.
constructor(
@Inject(DOCUMENT) doc: any, driver: AnimationDriver, normalizer: AnimationStyleNormalizer) {
@Inject(DOCUMENT) doc: any, driver: AnimationDriver, normalizer: AnimationStyleNormalizer,
appRef: ApplicationRef) {
super(doc.body, driver, normalizer);
}

Expand Down
Expand Up @@ -7,8 +7,9 @@
*/
import {animate, AnimationPlayer, AnimationTriggerMetadata, state, style, transition, trigger} from '@angular/animations';
import {ɵAnimationEngine as AnimationEngine} from '@angular/animations/browser';
import {Component, destroyPlatform, Injectable, NgModule, NgZone, RendererFactory2, RendererType2, ViewChild} from '@angular/core';
import {APP_INITIALIZER, Component, destroyPlatform, importProvidersFrom, Injectable, NgModule, NgZone, RendererFactory2, RendererType2, ViewChild} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {bootstrapApplication} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import {BrowserAnimationsModule, ɵAnimationRendererFactory as AnimationRendererFactory, ɵInjectableAnimationEngine as InjectableAnimationEngine} from '@angular/platform-browser/animations';
import {DomRendererFactory2} from '@angular/platform-browser/src/dom/dom_renderer';
Expand Down Expand Up @@ -330,7 +331,8 @@ describe('destroy', () => {
beforeEach(destroyPlatform);
afterEach(destroyPlatform);

it('should clear bootstrapped component contents',
// See https://github.com/angular/angular/issues/39955
it('should clear bootstrapped component contents when the `BrowserAnimationsModule` is imported',
withBody('<div>before</div><app-root></app-root><div>after</div>', async () => {
@Component({selector: 'app-root', template: 'app-root content'})
class AppComponent {
Expand All @@ -355,6 +357,99 @@ describe('destroy', () => {
expect(document.body.querySelector('app-root')).toBeFalsy(); // host element is removed
expect(document.body.childNodes.length).toEqual(2); // other elements are preserved
}));

// See https://github.com/angular/angular/issues/45108
it('should clear bootstrapped component contents when the animation engine is requested during initialization',
withBody('<div>before</div><app-root></app-root><div>after</div>', async () => {
@Injectable({providedIn: 'root'})
class AppService {
// The `RendererFactory2` is injected here explicitly because we import the
// `BrowserAnimationsModule`. The `RendererFactory2` will be provided with
// `AnimationRendererFactory` which relies on the `AnimationEngine`. We want to ensure that
// `ApplicationRef` is created earlier before the `AnimationEngine`, because previously the
// `AnimationEngine` was created before the `ApplicationRef` (see the link above to the
// original issue). The `ApplicationRef` was created after `APP_INITIALIZER` has been run
// but before the root module is bootstrapped.
constructor(rendererFactory: RendererFactory2) {}
}

@Component({selector: 'app-root', template: 'app-root content'})
class AppComponent {
}

@NgModule({
imports: [BrowserAnimationsModule],
declarations: [AppComponent],
bootstrap: [AppComponent],
providers: [
// The `APP_INITIALIZER` token is requested before the root module is bootstrapped, the
// `useFactory` just injects the `AppService` that injects the `RendererFactory2`.
{
provide: APP_INITIALIZER,
useFactory: () => (appService: AppService) => appService,
deps: [AppService],
multi: true
}
]
})
class AppModule {
}

const ngModuleRef = await platformBrowserDynamic().bootstrapModule(AppModule);

const root = document.body.querySelector('app-root')!;
expect(root.textContent).toEqual('app-root content');
expect(document.body.childNodes.length).toEqual(3);

ngModuleRef.destroy();

expect(document.body.querySelector('app-root')).toBeFalsy(); // host element is removed
expect(document.body.childNodes.length).toEqual(2); // other elements are preserved
}));

// See https://github.com/angular/angular/issues/45108
it('should clear standalone bootstrapped component contents when the animation engine is requested during initialization',
withBody('<div>before</div><app-root></app-root><div>after</div>', async () => {
@Injectable({providedIn: 'root'})
class AppService {
// The `RendererFactory2` is injected here explicitly because we import the
// `BrowserAnimationsModule`. The `RendererFactory2` will be provided with
// `AnimationRendererFactory` which relies on the `AnimationEngine`. We want to ensure
// that `ApplicationRef` is created earlier before the `AnimationEngine`, because
// previously the `AnimationEngine` was created before the `ApplicationRef` (see the link
// above to the original issue). The `ApplicationRef` was created after `APP_INITIALIZER`
// has been run but before the root module is bootstrapped.
constructor(rendererFactory: RendererFactory2) {}
}

@Component({selector: 'app-root', template: 'app-root content', standalone: true})
class AppComponent {
}

const appRef = await bootstrapApplication(AppComponent, {
providers: [
importProvidersFrom(BrowserAnimationsModule),
// The `APP_INITIALIZER` token is requested before the standalone component is
// bootstrapped, the `useFactory` just injects the `AppService` that injects the
// `RendererFactory2`.
{
provide: APP_INITIALIZER,
useFactory: () => (appService: AppService) => appService,
deps: [AppService],
multi: true
}
]
});

const root = document.body.querySelector('app-root')!;
expect(root.textContent).toEqual('app-root content');
expect(document.body.childNodes.length).toEqual(3);

appRef.destroy();

expect(document.body.querySelector('app-root')).toBeFalsy(); // host element is removed
expect(document.body.childNodes.length).toEqual(2); // other elements are
}));
});
})();

Expand Down

0 comments on commit 51be9bb

Please sign in to comment.