From fc1cd07eb002662c21177f28ef59714f3b2876ed 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 (#40120) 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 PR Close #40120 --- .../core/src/render3/instructions/listener.ts | 6 +- .../core/src/render3/instructions/shared.ts | 10 +- .../core/src/render3/node_manipulation.ts | 12 +- .../core/test/acceptance/component_spec.ts | 114 ++++++++++-------- .../bundling/forms/bundle.golden_symbols.json | 6 +- .../router/bundle.golden_symbols.json | 12 +- .../bundling/todo/bundle.golden_symbols.json | 6 +- 7 files changed, 91 insertions(+), 75 deletions(-) diff --git a/packages/core/src/render3/instructions/listener.ts b/packages/core/src/render3/instructions/listener.ts index 046787e22ac19..de3b2c6e35ae9 100644 --- a/packages/core/src/render3/instructions/listener.ts +++ b/packages/core/src/render3/instructions/listener.ts @@ -19,7 +19,7 @@ import {assertTNodeType} from '../node_assert'; import {getCurrentDirectiveDef, getCurrentTNode, getLView, getTView} from '../state'; import {getComponentLViewByIndex, getNativeByTNode, unwrapRNode} from '../util/view_utils'; -import {getLCleanup, getTViewCleanup, handleError, loadComponentRenderer, markViewDirty} from './shared'; +import {getOrCreateLViewCleanup, getOrCreateTViewCleanup, handleError, loadComponentRenderer, markViewDirty} from './shared'; @@ -120,12 +120,12 @@ function listenerInternal( eventTargetResolver?: GlobalTargetResolver): void { const isTNodeDirectiveHost = isDirectiveHost(tNode); const firstCreatePass = tView.firstCreatePass; - const tCleanup: false|any[] = firstCreatePass && getTViewCleanup(tView); + const tCleanup: false|any[] = firstCreatePass && getOrCreateTViewCleanup(tView); // When the ɵɵlistener instruction was generated and is executed we know that there is either a // native listener or a directive output on this element. As such we we know that we will have to // register a listener and store its cleanup function on LView. - const lCleanup = getLCleanup(lView); + const lCleanup = getOrCreateLViewCleanup(lView); ngDevMode && assertTNodeType(tNode, TNodeType.AnyRNode | TNodeType.AnyContainer); diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index f1a8c836085d6..9cb7bfe4cad36 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -759,19 +759,19 @@ export function locateHostElement( */ export function storeCleanupWithContext( tView: TView, lView: LView, context: any, cleanupFn: Function): void { - const lCleanup = getLCleanup(lView); + const lCleanup = getOrCreateLViewCleanup(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); } } } @@ -2006,12 +2006,12 @@ export function storePropertyBindingMetadata( export const CLEAN_PROMISE = _CLEAN_PROMISE; -export function getLCleanup(view: LView): any[] { +export function getOrCreateLViewCleanup(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 4b694a598af4c..5178445c4794b 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -480,12 +480,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', () => { diff --git a/packages/core/test/bundling/forms/bundle.golden_symbols.json b/packages/core/test/bundling/forms/bundle.golden_symbols.json index bf44a9ff83edb..1bf49380ee201 100644 --- a/packages/core/test/bundling/forms/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms/bundle.golden_symbols.json @@ -1019,9 +1019,6 @@ { "name": "getInjectorIndex" }, - { - "name": "getLCleanup" - }, { "name": "getLView" }, @@ -1052,6 +1049,9 @@ { "name": "getOrCreateInjectable" }, + { + "name": "getOrCreateLViewCleanup" + }, { "name": "getOrCreateNodeInjectorForNode" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index a6c8ad6d1330e..0f88828941602 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1334,9 +1334,6 @@ { "name": "getInjectorIndex" }, - { - "name": "getLCleanup" - }, { "name": "getLView" }, @@ -1367,6 +1364,9 @@ { "name": "getOrCreateInjectable" }, + { + "name": "getOrCreateLViewCleanup" + }, { "name": "getOrCreateNodeInjectorForNode" }, @@ -1376,6 +1376,9 @@ { "name": "getOrCreateTNode" }, + { + "name": "getOrCreateTViewCleanup" + }, { "name": "getOrCreateViewRefs" }, @@ -1442,9 +1445,6 @@ { "name": "getTView" }, - { - "name": "getTViewCleanup" - }, { "name": "getToken" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 839e1f4e85319..2062b31ee311a 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -377,9 +377,6 @@ { "name": "getInjectorIndex" }, - { - "name": "getLCleanup" - }, { "name": "getLView" }, @@ -404,6 +401,9 @@ { "name": "getOrCreateInjectable" }, + { + "name": "getOrCreateLViewCleanup" + }, { "name": "getOrCreateNodeInjectorForNode" },