fix(upgrade): correctly destroy nested downgraded component (#22400)

Previously, when a downgraded component was destroyed in a way that did
not trigger the `$destroy` event on the element (e.g. when a parent
element was removed from the DOM by Angular, not AngularJS), the
`ComponentRef` was not destroyed and unregistered.
This commit fixes it by listening for the `$destroy` event on both the
element and the scope.

Fixes #22392

PR Close #22400
This commit is contained in:
George Kalpakas 2018-02-23 15:18:21 +02:00 committed by Alex Eagle
parent 83b32a0a0a
commit 8a85888773
6 changed files with 121 additions and 10 deletions

View File

@ -209,12 +209,16 @@ export class DowngradeComponentAdapter {
registerCleanup() { registerCleanup() {
const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy());
let destroyed = false;
this.element.on !('$destroy', () => { this.element.on !('$destroy', () => this.componentScope.$destroy());
this.componentScope.$destroy(); this.componentScope.$on('$destroy', () => {
if (!destroyed) {
destroyed = true;
this.componentRef.injector.get(TestabilityRegistry) this.componentRef.injector.get(TestabilityRegistry)
.unregisterApplication(this.componentRef.location.nativeElement); .unregisterApplication(this.componentRef.location.nativeElement);
destroyComponentRef(); destroyComponentRef();
}
}); });
} }

View File

@ -263,6 +263,8 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck {
if (this.controllerInstance && isFunction(this.controllerInstance.$onDestroy)) { if (this.controllerInstance && isFunction(this.controllerInstance.$onDestroy)) {
this.controllerInstance.$onDestroy(); this.controllerInstance.$onDestroy();
} }
this.componentScope.$destroy();
} }
setComponentProperty(name: string, value: any) { setComponentProperty(name: string, value: any) {

View File

@ -85,15 +85,21 @@ withEachNg1Version(() => {
let element: angular.IAugmentedJQuery; let element: angular.IAugmentedJQuery;
class mockScope implements angular.IScope { class mockScope implements angular.IScope {
private destroyListeners: (() => void)[] = [];
$new() { return this; } $new() { return this; }
$watch(exp: angular.Ng1Expression, fn?: (a1?: any, a2?: any) => void) { $watch(exp: angular.Ng1Expression, fn?: (a1?: any, a2?: any) => void) {
return () => {}; return () => {};
} }
$on(event: string, fn?: (event?: any, ...args: any[]) => void) { $on(event: string, fn?: (event?: any, ...args: any[]) => void) {
if (event === '$destroy' && fn) {
this.destroyListeners.push(fn);
}
return () => {}; return () => {};
} }
$destroy() { $destroy() {
return () => {}; let listener: (() => void)|undefined;
while ((listener = this.destroyListeners.shift())) listener();
} }
$apply(exp?: angular.Ng1Expression) { $apply(exp?: angular.Ng1Expression) {
return () => {}; return () => {};

View File

@ -12,6 +12,11 @@ import {$ROOT_SCOPE} from '@angular/upgrade/src/common/constants';
export * from '../common/test_helpers'; export * from '../common/test_helpers';
export function $apply(adapter: UpgradeAdapterRef, exp: angular.Ng1Expression) {
const $rootScope = adapter.ng1Injector.get($ROOT_SCOPE) as angular.IRootScopeService;
$rootScope.$apply(exp);
}
export function $digest(adapter: UpgradeAdapterRef) { export function $digest(adapter: UpgradeAdapterRef) {
const $rootScope = adapter.ng1Injector.get($ROOT_SCOPE) as angular.IRootScopeService; const $rootScope = adapter.ng1Injector.get($ROOT_SCOPE) as angular.IRootScopeService;
$rootScope.$digest(); $rootScope.$digest();

View File

@ -6,13 +6,13 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import {ChangeDetectorRef, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgModuleFactory, NgZone, OnChanges, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core'; import {ChangeDetectorRef, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgModuleFactory, NgZone, OnChanges, OnDestroy, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core';
import {async, fakeAsync, flushMicrotasks, tick} from '@angular/core/testing'; import {async, fakeAsync, flushMicrotasks, tick} from '@angular/core/testing';
import {BrowserModule} from '@angular/platform-browser'; import {BrowserModule} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import * as angular from '@angular/upgrade/src/common/angular1'; import * as angular from '@angular/upgrade/src/common/angular1';
import {UpgradeAdapter, UpgradeAdapterRef} from '@angular/upgrade/src/dynamic/upgrade_adapter'; import {UpgradeAdapter, UpgradeAdapterRef} from '@angular/upgrade/src/dynamic/upgrade_adapter';
import {$digest, html, multiTrim, withEachNg1Version} from './test_helpers'; import {$apply, $digest, html, multiTrim, withEachNg1Version} from './test_helpers';
declare global { declare global {
export var inject: Function; export var inject: Function;
@ -582,6 +582,49 @@ withEachNg1Version(() => {
}); });
})); }));
it('should properly run cleanup with multiple levels of nesting', async(() => {
const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
let destroyed = false;
@Component(
{selector: 'ng2-outer', template: '<div *ngIf="!destroyIt"><ng1></ng1></div>'})
class Ng2OuterComponent {
@Input() destroyIt = false;
}
@Component({selector: 'ng2-inner', template: 'test'})
class Ng2InnerComponent implements OnDestroy {
ngOnDestroy() { destroyed = true; }
}
@NgModule({
imports: [BrowserModule],
declarations:
[Ng2InnerComponent, Ng2OuterComponent, adapter.upgradeNg1Component('ng1')],
schemas: [NO_ERRORS_SCHEMA],
})
class Ng2Module {
}
const ng1Module =
angular.module('ng1', [])
.directive('ng1', () => ({template: '<ng2-inner></ng2-inner>'}))
.directive('ng2Inner', adapter.downgradeNg2Component(Ng2InnerComponent))
.directive('ng2Outer', adapter.downgradeNg2Component(Ng2OuterComponent));
const element = html('<ng2-outer [destroy-it]="destroyIt"></ng2-outer>');
adapter.bootstrap(element, [ng1Module.name]).ready(ref => {
expect(element.textContent).toBe('test');
expect(destroyed).toBe(false);
$apply(ref, 'destroyIt = true');
expect(element.textContent).toBe('');
expect(destroyed).toBe(true);
});
}));
it('should fallback to the root ng2.injector when compiled outside the dom', async(() => { it('should fallback to the root ng2.injector when compiled outside the dom', async(() => {
const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
const ng1Module = angular.module('ng1', []); const ng1Module = angular.module('ng1', []);

View File

@ -6,11 +6,11 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import {ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core'; import {ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, Directive, ElementRef, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core';
import {async, fakeAsync, tick} from '@angular/core/testing'; import {async, fakeAsync, tick} from '@angular/core/testing';
import {BrowserModule} from '@angular/platform-browser'; import {BrowserModule} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import {UpgradeModule, downgradeComponent} from '@angular/upgrade/static'; import {UpgradeComponent, UpgradeModule, downgradeComponent} from '@angular/upgrade/static';
import * as angular from '@angular/upgrade/static/src/common/angular1'; import * as angular from '@angular/upgrade/static/src/common/angular1';
import {$apply, bootstrap, html, multiTrim, withEachNg1Version} from '../test_helpers'; import {$apply, bootstrap, html, multiTrim, withEachNg1Version} from '../test_helpers';
@ -461,6 +461,57 @@ withEachNg1Version(() => {
}); });
})); }));
it('should properly run cleanup with multiple levels of nesting', async(() => {
let destroyed = false;
@Component({
selector: 'ng2-outer',
template: '<div *ngIf="!destroyIt"><ng1></ng1></div>',
})
class Ng2OuterComponent {
@Input() destroyIt = false;
}
@Component({selector: 'ng2-inner', template: 'test'})
class Ng2InnerComponent implements OnDestroy {
ngOnDestroy() { destroyed = true; }
}
@Directive({selector: 'ng1'})
class Ng1ComponentFacade extends UpgradeComponent {
constructor(elementRef: ElementRef, injector: Injector) {
super('ng1', elementRef, injector);
}
}
@NgModule({
imports: [BrowserModule, UpgradeModule],
declarations: [Ng1ComponentFacade, Ng2InnerComponent, Ng2OuterComponent],
entryComponents: [Ng2InnerComponent, Ng2OuterComponent],
})
class Ng2Module {
ngDoBootstrap() {}
}
const ng1Module =
angular.module('ng1', [])
.directive('ng1', () => ({template: '<ng2-inner></ng2-inner>'}))
.directive('ng2Inner', downgradeComponent({component: Ng2InnerComponent}))
.directive('ng2Outer', downgradeComponent({component: Ng2OuterComponent}));
const element = html('<ng2-outer [destroy-it]="destroyIt"></ng2-outer>');
bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => {
expect(element.textContent).toBe('test');
expect(destroyed).toBe(false);
$apply(upgrade, 'destroyIt = true');
expect(element.textContent).toBe('');
expect(destroyed).toBe(true);
});
}));
it('should work when compiled outside the dom (by fallback to the root ng2.injector)', it('should work when compiled outside the dom (by fallback to the root ng2.injector)',
async(() => { async(() => {