From e3c0cb3cf518006fc693a470231df98decd4bd9c Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Mon, 14 Dec 2020 14:14:00 -0800 Subject: [PATCH] fix(core): Call `onDestroy` in production mode as well PR #39876 introduced an error where the `onDestroy` of `ComponentRef` would only get called if `ngDevMode` was set to true. This was because in dev mode we would freeze `TCleanup` to verify that no more static cleanup would get added to `TCleanup` array. This ensured that `TCleanup` was always present in dev mode. In production the `TCleanup` would get created only when needed. The resulting cleanup code was incorrectly indented and would only run if `TCleanup` was present causing this issue. Fix #40105 --- .../core/src/render3/instructions/shared.ts | 10 +- .../core/src/render3/node_manipulation.ts | 12 +- .../core/test/acceptance/component_spec.ts | 114 ++++++++++-------- 3 files changed, 76 insertions(+), 60 deletions(-) diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index c0b11b180a1e5..89fa9d07fd220 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -758,19 +758,19 @@ export function locateHostElement( */ export function storeCleanupWithContext( tView: TView, lView: LView, context: any, cleanupFn: Function): void { - const lCleanup = getLCleanup(lView); + const lCleanup = getOrCreateLCleanup(lView); if (context === null) { // If context is null that this is instance specific callback. These callbacks can only be // inserted after template shared instances. For this reason in ngDevMode we freeze the TView. if (ngDevMode) { - Object.freeze(getTViewCleanup(tView)); + Object.freeze(getOrCreateTViewCleanup(tView)); } lCleanup.push(cleanupFn); } else { lCleanup.push(context); if (tView.firstCreatePass) { - getTViewCleanup(tView).push(cleanupFn, lCleanup.length - 1); + getOrCreateTViewCleanup(tView).push(cleanupFn, lCleanup.length - 1); } } } @@ -2004,12 +2004,12 @@ export function storePropertyBindingMetadata( export const CLEAN_PROMISE = _CLEAN_PROMISE; -export function getLCleanup(view: LView): any[] { +export function getOrCreateLCleanup(view: LView): any[] { // top level variables should not be exported for performance reasons (PERF_NOTES.md) return view[CLEANUP] || (view[CLEANUP] = ngDevMode ? new LCleanup() : []); } -export function getTViewCleanup(tView: TView): any[] { +export function getOrCreateTViewCleanup(tView: TView): any[] { return tView.cleanup || (tView.cleanup = ngDevMode ? new TCleanup() : []); } diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 8a8cdbe1bae7c..0ec115e10f798 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -479,12 +479,12 @@ function processCleanups(tView: TView, lView: LView): void { tCleanup[i].call(context); } } - if (lCleanup !== null) { - for (let i = lastLCleanupIndex + 1; i < lCleanup.length; i++) { - const instanceCleanupFn = lCleanup[i]; - ngDevMode && assertFunction(instanceCleanupFn, 'Expecting instance cleanup function.'); - instanceCleanupFn(); - } + } + if (lCleanup !== null) { + for (let i = lastLCleanupIndex + 1; i < lCleanup.length; i++) { + const instanceCleanupFn = lCleanup[i]; + ngDevMode && assertFunction(instanceCleanupFn, 'Expecting instance cleanup function.'); + instanceCleanupFn(); } lView[CLEANUP] = null; } diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts index 463759577107d..41c8208b78294 100644 --- a/packages/core/test/acceptance/component_spec.ts +++ b/packages/core/test/acceptance/component_spec.ts @@ -303,55 +303,71 @@ describe('component', () => { expect(wrapperEls.length).toBe(2); // other elements are preserved }); - it('should invoke `onDestroy` callbacks of dynamically created component', () => { - let wasOnDestroyCalled = false; - @Component({ - selector: '[comp]', - template: 'comp content', - }) - class DynamicComponent { - } - - @NgModule({ - declarations: [DynamicComponent], - entryComponents: [DynamicComponent], // needed only for ViewEngine - }) - class TestModule { - } - - @Component({ - selector: 'button', - template: '
', - }) - class App { - @ViewChild('anchor', {read: ViewContainerRef}) anchor!: ViewContainerRef; - - constructor(private cfr: ComponentFactoryResolver, private injector: Injector) {} - - create() { - const factory = this.cfr.resolveComponentFactory(DynamicComponent); - const componentRef = factory.create(this.injector); - componentRef.onDestroy(() => { - wasOnDestroyCalled = true; - }); - this.anchor.insert(componentRef.hostView); - } - - clear() { - this.anchor.clear(); - } - } - - TestBed.configureTestingModule({imports: [TestModule], declarations: [App]}); - const fixture = TestBed.createComponent(App); - fixture.detectChanges(); - - // Add ComponentRef to ViewContainerRef instance. - fixture.componentInstance.create(); - // Clear ViewContainerRef to invoke `onDestroy` callbacks on ComponentRef. - fixture.componentInstance.clear(); - - expect(wasOnDestroyCalled).toBeTrue(); + describe('with ngDevMode', () => { + const _global: {ngDevMode: any} = global as any; + let saveNgDevMode!: typeof ngDevMode; + beforeEach(() => saveNgDevMode = ngDevMode); + afterEach(() => _global.ngDevMode = saveNgDevMode); + // In dev mode we have some additional logic to freeze `TView.cleanup` array + // (see `storeCleanupWithContext` function). + // The tests below verify that this action doesn't trigger any change in behaviour + // for prod mode. See https://github.com/angular/angular/issues/40105. + ['ngDevMode off', 'ngDevMode on'].forEach((mode) => { + it('should invoke `onDestroy` callbacks of dynamically created component with ' + mode, + () => { + if (mode === 'ngDevMode off') { + _global.ngDevMode = false; + } + let wasOnDestroyCalled = false; + @Component({ + selector: '[comp]', + template: 'comp content', + }) + class DynamicComponent { + } + + @NgModule({ + declarations: [DynamicComponent], + entryComponents: [DynamicComponent], // needed only for ViewEngine + }) + class TestModule { + } + + @Component({ + selector: 'button', + template: '
', + }) + class App { + @ViewChild('anchor', {read: ViewContainerRef}) anchor!: ViewContainerRef; + + constructor(private cfr: ComponentFactoryResolver, private injector: Injector) {} + + create() { + const factory = this.cfr.resolveComponentFactory(DynamicComponent); + const componentRef = factory.create(this.injector); + componentRef.onDestroy(() => { + wasOnDestroyCalled = true; + }); + this.anchor.insert(componentRef.hostView); + } + + clear() { + this.anchor.clear(); + } + } + + TestBed.configureTestingModule({imports: [TestModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + // Add ComponentRef to ViewContainerRef instance. + fixture.componentInstance.create(); + // Clear ViewContainerRef to invoke `onDestroy` callbacks on ComponentRef. + fixture.componentInstance.clear(); + + expect(wasOnDestroyCalled).toBeTrue(); + }); + }); }); describe('invalid host element', () => {