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
This commit is contained in:
Misko Hevery 2020-12-14 14:14:00 -08:00 committed by Joey Perrott
parent 5ba7bcd2a6
commit fc1cd07eb0
7 changed files with 83 additions and 67 deletions

View File

@ -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);

View File

@ -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() : []);
}

View File

@ -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;
}

View File

@ -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 {
}
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 {
}
@NgModule({
declarations: [DynamicComponent],
entryComponents: [DynamicComponent], // needed only for ViewEngine
})
class TestModule {
}
@Component({
selector: 'button',
template: '<div id="app-root" #anchor></div>',
})
class App {
@ViewChild('anchor', {read: ViewContainerRef}) anchor!: ViewContainerRef;
@Component({
selector: 'button',
template: '<div id="app-root" #anchor></div>',
})
class App {
@ViewChild('anchor', {read: ViewContainerRef}) anchor!: ViewContainerRef;
constructor(private cfr: ComponentFactoryResolver, private injector: Injector) {}
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);
}
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();
}
}
clear() {
this.anchor.clear();
}
}
TestBed.configureTestingModule({imports: [TestModule], declarations: [App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
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();
// Add ComponentRef to ViewContainerRef instance.
fixture.componentInstance.create();
// Clear ViewContainerRef to invoke `onDestroy` callbacks on ComponentRef.
fixture.componentInstance.clear();
expect(wasOnDestroyCalled).toBeTrue();
expect(wasOnDestroyCalled).toBeTrue();
});
});
});
describe('invalid host element', () => {

View File

@ -1019,9 +1019,6 @@
{
"name": "getInjectorIndex"
},
{
"name": "getLCleanup"
},
{
"name": "getLView"
},
@ -1052,6 +1049,9 @@
{
"name": "getOrCreateInjectable"
},
{
"name": "getOrCreateLViewCleanup"
},
{
"name": "getOrCreateNodeInjectorForNode"
},

View File

@ -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"
},

View File

@ -377,9 +377,6 @@
{
"name": "getInjectorIndex"
},
{
"name": "getLCleanup"
},
{
"name": "getLView"
},
@ -404,6 +401,9 @@
{
"name": "getOrCreateInjectable"
},
{
"name": "getOrCreateLViewCleanup"
},
{
"name": "getOrCreateNodeInjectorForNode"
},