fix(upgrade): correctly destroy nested downgraded component (angular#…

…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 angular#22392

PR Close angular#22400

packages/upgrade/src/common/downgrade_component_adapter.ts

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

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) {

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 () => {};

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

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: '<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(() => {
            const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
            const ng1Module = angular.module('ng1', []);

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: '<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)',
        async(() => {