Skip to content

Commit

Permalink
fix(core): Call onDestroy in production mode as well (#40120)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@mhevery @josephperrott
mhevery authored and josephperrott committed Dec 22, 2020
1 parent 5ba7bcd commit fc1cd07
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 75 deletions.
6 changes: 3 additions & 3 deletions packages/core/src/render3/instructions/listener.ts
View file
Expand Up @@ -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';



Expand Down Expand Up @@ -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);

Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/render3/instructions/shared.ts
View file
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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() : []);
}

Expand Down
12 changes: 6 additions & 6 deletions packages/core/src/render3/node_manipulation.ts
View file
Expand Up @@ -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;
}
Expand Down
114 changes: 65 additions & 49 deletions packages/core/test/acceptance/component_spec.ts
View file
Expand Up @@ -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: '<div id="app-root" #anchor></div>',
})
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: '<div id="app-root" #anchor></div>',
})
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', () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/forms/bundle.golden_symbols.json
View file
Expand Up @@ -1019,9 +1019,6 @@
{
"name": "getInjectorIndex"
},
{
"name": "getLCleanup"
},
{
"name": "getLView"
},
Expand Down Expand Up @@ -1052,6 +1049,9 @@
{
"name": "getOrCreateInjectable"
},
{
"name": "getOrCreateLViewCleanup"
},
{
"name": "getOrCreateNodeInjectorForNode"
},
Expand Down
12 changes: 6 additions & 6 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
View file
Expand Up @@ -1334,9 +1334,6 @@
{
"name": "getInjectorIndex"
},
{
"name": "getLCleanup"
},
{
"name": "getLView"
},
Expand Down Expand Up @@ -1367,6 +1364,9 @@
{
"name": "getOrCreateInjectable"
},
{
"name": "getOrCreateLViewCleanup"
},
{
"name": "getOrCreateNodeInjectorForNode"
},
Expand All @@ -1376,6 +1376,9 @@
{
"name": "getOrCreateTNode"
},
{
"name": "getOrCreateTViewCleanup"
},
{
"name": "getOrCreateViewRefs"
},
Expand Down Expand Up @@ -1442,9 +1445,6 @@
{
"name": "getTView"
},
{
"name": "getTViewCleanup"
},
{
"name": "getToken"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
View file
Expand Up @@ -377,9 +377,6 @@
{
"name": "getInjectorIndex"
},
{
"name": "getLCleanup"
},
{
"name": "getLView"
},
Expand All @@ -404,6 +401,9 @@
{
"name": "getOrCreateInjectable"
},
{
"name": "getOrCreateLViewCleanup"
},
{
"name": "getOrCreateNodeInjectorForNode"
},
Expand Down

0 comments on commit fc1cd07

Please sign in to comment.