fix(core): cleanup DOM elements when root view is removed (#37600)

Currently when bootstrapped component is being removed using `ComponentRef.destroy` or `NgModuleRef.destroy` methods, DOM nodes may be retained in the DOM tree. This commit fixes that problem by always attaching host element of the internal root view to the component's host view node, so the cleanup can happen correctly.

Resolves #36449.

PR Close #37600
This commit is contained in:
Andrew Kushnir 2020-06-15 18:08:46 -07:00
parent d37049a2a2
commit 9118f49a63
5 changed files with 166 additions and 59 deletions

View File

@ -155,14 +155,6 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
const rootFlags = this.componentDef.onPush ? LViewFlags.Dirty | LViewFlags.IsRoot :
LViewFlags.CheckAlways | LViewFlags.IsRoot;
// Check whether this Component needs to be isolated from other components, i.e. whether it
// should be placed into its own (empty) root context or existing root context should be used.
// Note: this is internal-only convention and might change in the future, so it should not be
// relied upon externally.
const isIsolated = typeof rootSelectorOrNode === 'string' &&
/^#root-ng-internal-isolated-\d+/.test(rootSelectorOrNode);
const rootContext = createRootContext();
// Create the root view. Uses empty TView and ContentTemplate.
@ -232,12 +224,10 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
this.componentType, component,
createElementRef(viewEngine_ElementRef, tElementNode, rootLView), rootLView, tElementNode);
if (!rootSelectorOrNode || isIsolated) {
// The host element of the internal or isolated root view is attached to the component's host
// view node.
ngDevMode && assertNodeOfPossibleTypes(rootTView.node, TNodeType.View);
rootTView.node!.child = tElementNode;
}
// The host element of the internal root view is attached to the component's host view node.
ngDevMode && assertNodeOfPossibleTypes(rootTView.node, TNodeType.View);
rootTView.node!.child = tElementNode;
return componentRef;
}
}

View File

@ -40,9 +40,11 @@ describe('bootstrap', () => {
describe('options', () => {
function createComponentAndModule(
options: {encapsulation?: ViewEncapsulation; preserveWhitespaces?: boolean} = {}) {
options:
{encapsulation?: ViewEncapsulation; preserveWhitespaces?: boolean;
selector?: string} = {}) {
@Component({
selector: 'my-app',
selector: options.selector || 'my-app',
styles: [''],
template: '<span>a b</span>',
encapsulation: options.encapsulation,
@ -155,16 +157,17 @@ describe('bootstrap', () => {
});
it('should log an error when changing defaultEncapsulation bootstrap options',
withBody('<my-app></my-app>', async () => {
const TestModule = createComponentAndModule();
withBody('<my-app-a></my-app-a><my-app-b></my-app-b>', async () => {
const platformRef = platformBrowserDynamic();
const ngModuleRef = await platformRef.bootstrapModule(
TestModule, {defaultEncapsulation: ViewEncapsulation.None});
ngModuleRef.destroy();
const TestModuleA = createComponentAndModule({selector: 'my-app-a'});
const ngModuleRefA = await platformRef.bootstrapModule(
TestModuleA, {defaultEncapsulation: ViewEncapsulation.None});
ngModuleRefA.destroy();
const ngModuleRef2 = await platformRef.bootstrapModule(
TestModule, {defaultEncapsulation: ViewEncapsulation.ShadowDom});
const TestModuleB = createComponentAndModule({selector: 'my-app-b'});
const ngModuleRefB = await platformRef.bootstrapModule(
TestModuleB, {defaultEncapsulation: ViewEncapsulation.ShadowDom});
expect(console.error)
.toHaveBeenCalledWith(
'Provided value for `defaultEncapsulation` can not be changed once it has been set.');
@ -172,20 +175,21 @@ describe('bootstrap', () => {
// The options should not have been changed
expect(document.body.innerHTML).not.toContain('_ngcontent-');
ngModuleRef2.destroy();
ngModuleRefB.destroy();
}));
it('should log an error when changing preserveWhitespaces bootstrap options',
withBody('<my-app></my-app>', async () => {
const TestModule = createComponentAndModule();
withBody('<my-app-a></my-app-a><my-app-b></my-app-b>', async () => {
const platformRef = platformBrowserDynamic();
const ngModuleRef =
await platformRef.bootstrapModule(TestModule, {preserveWhitespaces: true});
ngModuleRef.destroy();
const TestModuleA = createComponentAndModule({selector: 'my-app-a'});
const ngModuleRefA =
await platformRef.bootstrapModule(TestModuleA, {preserveWhitespaces: true});
ngModuleRefA.destroy();
const ngModuleRef2 =
await platformRef.bootstrapModule(TestModule, {preserveWhitespaces: false});
const TestModuleB = createComponentAndModule({selector: 'my-app-b'});
const ngModuleRefB =
await platformRef.bootstrapModule(TestModuleB, {preserveWhitespaces: false});
expect(console.error)
.toHaveBeenCalledWith(
'Provided value for `preserveWhitespaces` can not be changed once it has been set.');
@ -193,62 +197,65 @@ describe('bootstrap', () => {
// The options should not have been changed
expect(document.body.innerHTML).toContain('a b');
ngModuleRef2.destroy();
ngModuleRefB.destroy();
}));
it('should log an error when changing defaultEncapsulation to its default',
withBody('<my-app></my-app>', async () => {
const TestModule = createComponentAndModule();
withBody('<my-app-a></my-app-a><my-app-b></my-app-b>', async () => {
const platformRef = platformBrowserDynamic();
const ngModuleRef = await platformRef.bootstrapModule(TestModule);
ngModuleRef.destroy();
const TestModuleA = createComponentAndModule({selector: 'my-app-a'});
const ngModuleRefA = await platformRef.bootstrapModule(TestModuleA);
ngModuleRefA.destroy();
const ngModuleRef2 = await platformRef.bootstrapModule(
TestModule, {defaultEncapsulation: ViewEncapsulation.Emulated});
const TestModuleB = createComponentAndModule({selector: 'my-app-b'});
const ngModuleRefB = await platformRef.bootstrapModule(
TestModuleB, {defaultEncapsulation: ViewEncapsulation.Emulated});
// Although the configured value may be identical to the default, the provided set of
// options has still been changed compared to the previously provided options.
expect(console.error)
.toHaveBeenCalledWith(
'Provided value for `defaultEncapsulation` can not be changed once it has been set.');
ngModuleRef2.destroy();
ngModuleRefB.destroy();
}));
it('should log an error when changing preserveWhitespaces to its default',
withBody('<my-app></my-app>', async () => {
const TestModule = createComponentAndModule();
withBody('<my-app-a></my-app-a><my-app-b></my-app-b>', async () => {
const platformRef = platformBrowserDynamic();
const ngModuleRef = await platformRef.bootstrapModule(TestModule);
ngModuleRef.destroy();
const TestModuleA = createComponentAndModule({selector: 'my-app-a'});
const ngModuleRefA = await platformRef.bootstrapModule(TestModuleA);
ngModuleRefA.destroy();
const ngModuleRef2 =
await platformRef.bootstrapModule(TestModule, {preserveWhitespaces: false});
const TestModuleB = createComponentAndModule({selector: 'my-app-b'});
const ngModuleRefB =
await platformRef.bootstrapModule(TestModuleB, {preserveWhitespaces: false});
// Although the configured value may be identical to the default, the provided set of
// options has still been changed compared to the previously provided options.
expect(console.error)
.toHaveBeenCalledWith(
'Provided value for `preserveWhitespaces` can not be changed once it has been set.');
ngModuleRef2.destroy();
ngModuleRefB.destroy();
}));
it('should not log an error when passing identical bootstrap options',
withBody('<my-app></my-app>', async () => {
const TestModule = createComponentAndModule();
withBody('<my-app-a></my-app-a><my-app-b></my-app-b>', async () => {
const platformRef = platformBrowserDynamic();
const ngModuleRef1 = await platformRef.bootstrapModule(
TestModule,
const TestModuleA = createComponentAndModule({selector: 'my-app-a'});
const ngModuleRefA = await platformRef.bootstrapModule(
TestModuleA,
{defaultEncapsulation: ViewEncapsulation.None, preserveWhitespaces: true});
ngModuleRef1.destroy();
ngModuleRefA.destroy();
// Bootstrapping multiple modules using the exact same options should be allowed.
const ngModuleRef2 = await platformRef.bootstrapModule(
TestModule,
const TestModuleB = createComponentAndModule({selector: 'my-app-b'});
const ngModuleRefB = await platformRef.bootstrapModule(
TestModuleB,
{defaultEncapsulation: ViewEncapsulation.None, preserveWhitespaces: true});
ngModuleRef2.destroy();
ngModuleRefB.destroy();
}));
});
});
@ -282,4 +289,4 @@ export class MultipleSelectorsAppComponent {
bootstrap: [MultipleSelectorsAppComponent],
})
export class MultipleSelectorsAppModule {
}
}

View File

@ -7,7 +7,7 @@
*/
import {DOCUMENT} from '@angular/common';
import {Component, ComponentFactoryResolver, ComponentRef, ElementRef, InjectionToken, Injector, Input, NgModule, OnDestroy, Renderer2, RendererFactory2, Type, ViewChild, ViewContainerRef, ViewEncapsulation, ɵsetDocument} from '@angular/core';
import {ApplicationRef, Component, ComponentFactoryResolver, ComponentRef, ElementRef, InjectionToken, Injector, Input, NgModule, OnDestroy, Renderer2, RendererFactory2, Type, ViewChild, ViewContainerRef, ViewEncapsulation, ɵsetDocument} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser';
import {expect} from '@angular/platform-browser/testing/src/matchers';
@ -189,6 +189,76 @@ describe('component', () => {
});
});
it('should clear the contents of dynamically created component when it\'s attached to ApplicationRef',
() => {
let wasOnDestroyCalled = false;
@Component({
selector: '[comp]',
template: 'comp content',
})
class DynamicComponent {
ngOnDestroy() {
wasOnDestroyCalled = true;
}
}
@NgModule({
declarations: [DynamicComponent],
entryComponents: [DynamicComponent], // needed only for ViewEngine
})
class TestModule {
}
@Component({
selector: 'button',
template: `
<div class="wrapper"></div>
<div id="app-root"></div>
<div class="wrapper"></div>
`,
})
class App {
componentRef!: ComponentRef<DynamicComponent>;
constructor(
private cfr: ComponentFactoryResolver, private injector: Injector,
private appRef: ApplicationRef) {}
create() {
const factory = this.cfr.resolveComponentFactory(DynamicComponent);
// Component to be bootstrapped into an element with the `app-root` id.
this.componentRef = factory.create(this.injector, undefined, '#app-root');
this.appRef.attachView(this.componentRef.hostView);
}
destroy() {
this.componentRef.destroy();
}
}
TestBed.configureTestingModule({imports: [TestModule], declarations: [App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
let appRootEl = fixture.nativeElement.querySelector('#app-root');
expect(appRootEl).toBeDefined();
expect(appRootEl.innerHTML).toBe(''); // app container content is empty
fixture.componentInstance.create();
appRootEl = fixture.nativeElement.querySelector('#app-root');
expect(appRootEl).toBeDefined();
expect(appRootEl.innerHTML).toBe('comp content');
fixture.componentInstance.destroy();
fixture.detectChanges();
appRootEl = fixture.nativeElement.querySelector('#app-root');
expect(appRootEl).toBeFalsy(); // host element is removed
const wrapperEls = fixture.nativeElement.querySelectorAll('.wrapper');
expect(wrapperEls.length).toBe(2); // other elements are preserved
});
it('should use a new ngcontent attribute for child elements created w/ Renderer2', () => {
@Component({
selector: 'app-root',

View File

@ -7,10 +7,12 @@
*/
import {CommonModule} from '@angular/common';
import {Component, CUSTOM_ELEMENTS_SCHEMA, Injectable, InjectionToken, NgModule, NgModuleRef, NO_ERRORS_SCHEMA, ɵsetClassMetadata as setClassMetadata, ɵɵdefineComponent as defineComponent, ɵɵdefineInjector as defineInjector, ɵɵdefineNgModule as defineNgModule, ɵɵelement as element, ɵɵproperty as property} from '@angular/core';
import {Component, CUSTOM_ELEMENTS_SCHEMA, destroyPlatform, Injectable, InjectionToken, NgModule, NgModuleRef, NO_ERRORS_SCHEMA, ɵsetClassMetadata as setClassMetadata, ɵɵdefineComponent as defineComponent, ɵɵdefineInjector as defineInjector, ɵɵdefineNgModule as defineNgModule, ɵɵelement as element, ɵɵproperty as property} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {BrowserModule} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import {expect} from '@angular/platform-browser/testing/src/matchers';
import {modifiedInIvy, onlyInIvy} from '@angular/private/testing';
import {modifiedInIvy, onlyInIvy, withBody} from '@angular/private/testing';
describe('NgModule', () => {
@Component({template: 'hello'})
@ -100,6 +102,44 @@ describe('NgModule', () => {
expect(TestBed.inject(Service).initializations).toEqual(['RoutesModule', 'AppModule']);
});
describe('destroy', () => {
beforeEach(destroyPlatform);
afterEach(destroyPlatform);
it('should clear bootstrapped component contents',
withBody('<div>before</div><button></button><div>after</div>', async () => {
let wasOnDestroyCalled = false;
@Component({
selector: 'button',
template: 'button content',
})
class App {
ngOnDestroy() {
wasOnDestroyCalled = true;
}
}
@NgModule({
imports: [BrowserModule],
declarations: [App],
bootstrap: [App],
})
class AppModule {
}
const ngModuleRef = await platformBrowserDynamic().bootstrapModule(AppModule);
const button = document.body.querySelector('button')!;
expect(button.textContent).toEqual('button content');
expect(document.body.childNodes.length).toEqual(3);
ngModuleRef.destroy();
expect(wasOnDestroyCalled).toEqual(true);
expect(document.body.querySelector('button')).toBeFalsy(); // host element is removed
expect(document.body.childNodes.length).toEqual(2); // other elements are preserved
}));
});
describe('schemas', () => {
onlyInIvy('Unknown property logs an error message instead of throwing')
.it('should throw on unknown props if NO_ERRORS_SCHEMA is absent', () => {

View File

@ -332,7 +332,7 @@ export class TestBedRender3 implements TestBed {
createComponent<T>(type: Type<T>): ComponentFixture<T> {
const testComponentRenderer = this.inject(TestComponentRenderer);
const rootElId = `root-ng-internal-isolated-${_nextRootElementId++}`;
const rootElId = `root${_nextRootElementId++}`;
testComponentRenderer.insertRootElement(rootElId);
const componentDef = (type as any).ɵcmp;