From 8a85888773580527cbb9eddd2cc042931e3bdd47 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 23 Feb 2018 15:18:21 +0200 Subject: [PATCH] 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 --- .../src/common/downgrade_component_adapter.ts | 14 +++-- .../src/dynamic/upgrade_ng1_adapter.ts | 2 + .../downgrade_component_adapter_spec.ts | 8 ++- packages/upgrade/test/dynamic/test_helpers.ts | 5 ++ packages/upgrade/test/dynamic/upgrade_spec.ts | 47 +++++++++++++++- .../integration/downgrade_component_spec.ts | 55 ++++++++++++++++++- 6 files changed, 121 insertions(+), 10 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index 4e44bfcf9f..6ba0238675 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -209,12 +209,16 @@ export class DowngradeComponentAdapter { registerCleanup() { const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); + let destroyed = false; - this.element.on !('$destroy', () => { - this.componentScope.$destroy(); - this.componentRef.injector.get(TestabilityRegistry) - .unregisterApplication(this.componentRef.location.nativeElement); - destroyComponentRef(); + this.element.on !('$destroy', () => this.componentScope.$destroy()); + this.componentScope.$on('$destroy', () => { + if (!destroyed) { + destroyed = true; + this.componentRef.injector.get(TestabilityRegistry) + .unregisterApplication(this.componentRef.location.nativeElement); + destroyComponentRef(); + } }); } diff --git a/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts b/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts index 06d9ed44dc..81f21e6a1c 100644 --- a/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts +++ b/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts @@ -263,6 +263,8 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck { if (this.controllerInstance && isFunction(this.controllerInstance.$onDestroy)) { this.controllerInstance.$onDestroy(); } + + this.componentScope.$destroy(); } setComponentProperty(name: string, value: any) { diff --git a/packages/upgrade/test/common/downgrade_component_adapter_spec.ts b/packages/upgrade/test/common/downgrade_component_adapter_spec.ts index d7110815ba..c428c24bd8 100644 --- a/packages/upgrade/test/common/downgrade_component_adapter_spec.ts +++ b/packages/upgrade/test/common/downgrade_component_adapter_spec.ts @@ -85,15 +85,21 @@ withEachNg1Version(() => { let element: angular.IAugmentedJQuery; class mockScope implements angular.IScope { + private destroyListeners: (() => void)[] = []; + $new() { return this; } $watch(exp: angular.Ng1Expression, fn?: (a1?: any, a2?: any) => void) { return () => {}; } $on(event: string, fn?: (event?: any, ...args: any[]) => void) { + if (event === '$destroy' && fn) { + this.destroyListeners.push(fn); + } return () => {}; } $destroy() { - return () => {}; + let listener: (() => void)|undefined; + while ((listener = this.destroyListeners.shift())) listener(); } $apply(exp?: angular.Ng1Expression) { return () => {}; diff --git a/packages/upgrade/test/dynamic/test_helpers.ts b/packages/upgrade/test/dynamic/test_helpers.ts index 284ef8bb6f..7723ecc361 100644 --- a/packages/upgrade/test/dynamic/test_helpers.ts +++ b/packages/upgrade/test/dynamic/test_helpers.ts @@ -12,6 +12,11 @@ import {$ROOT_SCOPE} from '@angular/upgrade/src/common/constants'; 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) { const $rootScope = adapter.ng1Injector.get($ROOT_SCOPE) as angular.IRootScopeService; $rootScope.$digest(); diff --git a/packages/upgrade/test/dynamic/upgrade_spec.ts b/packages/upgrade/test/dynamic/upgrade_spec.ts index ae3044b89a..48d3412834 100644 --- a/packages/upgrade/test/dynamic/upgrade_spec.ts +++ b/packages/upgrade/test/dynamic/upgrade_spec.ts @@ -6,13 +6,13 @@ * 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 {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import * as angular from '@angular/upgrade/src/common/angular1'; 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 { 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: '
'}) + 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: ''})) + .directive('ng2Inner', adapter.downgradeNg2Component(Ng2InnerComponent)) + .directive('ng2Outer', adapter.downgradeNg2Component(Ng2OuterComponent)); + + const element = html(''); + + 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(() => { const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); const ng1Module = angular.module('ng1', []); diff --git a/packages/upgrade/test/static/integration/downgrade_component_spec.ts b/packages/upgrade/test/static/integration/downgrade_component_spec.ts index 6a7b4affcc..2f977c78e8 100644 --- a/packages/upgrade/test/static/integration/downgrade_component_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_component_spec.ts @@ -6,11 +6,11 @@ * 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 {BrowserModule} from '@angular/platform-browser'; 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 {$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: '
', + }) + 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: ''})) + .directive('ng2Inner', downgradeComponent({component: Ng2InnerComponent})) + .directive('ng2Outer', downgradeComponent({component: Ng2OuterComponent})); + + const element = html(''); + + 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)', async(() => {