fix(upgrade): properly destroy upgraded component elements and descendants (#26209)

Fixes #26208 PR Close #26209
2018-10-01 21:29:38 -07:00 · 2018-10-01 21:29:38 -07:00 · 5a31bde649
commit 5a31bde649
parent decc0b840d
4 changed files with 307 additions and 9 deletions
--- a/packages/upgrade/src/common/angular1.ts
+++ b/packages/upgrade/src/common/angular1.ts
@ -223,14 +223,17 @@ let angular: {
  bootstrap: (e: Element, modules: (string | IInjectable)[], config?: IAngularBootstrapConfig) =>
                 IInjectorService,
  module: (prefix: string, dependencies?: string[]) => IModule,
-  element: (e: string | Element | Document | IAugmentedJQuery) => IAugmentedJQuery,
+  element: {
+    (e: string | Element | Document | IAugmentedJQuery): IAugmentedJQuery;
+    cleanData: (nodes: Node[] | NodeList) => void;
+  },
  version: {major: number},
  resumeBootstrap: () => void,
  getTestability: (e: Element) => ITestabilityService
 } = <any>{
  bootstrap: noNg,
  module: noNg,
-  element: noNg,
+  element: Object.assign(() => noNg(), {cleanData: noNg}),
  version: undefined,
  resumeBootstrap: noNg,
  getTestability: noNg
@ -281,7 +284,9 @@ export const bootstrap: typeof angular.bootstrap = (e, modules, config?) =>
 export const module: typeof angular.module = (prefix, dependencies?) =>
    angular.module(prefix, dependencies);

-export const element: typeof angular.element = e => angular.element(e);
+export const element: typeof angular.element = Object.assign(
+    (e: string | Element | Document | IAugmentedJQuery) => angular.element(e),
+    {cleanData: (nodes: Node[] | NodeList) => angular.element.cleanData(nodes)});

 export const resumeBootstrap: typeof angular.resumeBootstrap = () => angular.resumeBootstrap();

--- a/packages/upgrade/src/common/upgrade_helper.ts
+++ b/packages/upgrade/src/common/upgrade_helper.ts
@ -124,7 +124,15 @@ export class UpgradeHelper {
      controllerInstance.$onDestroy();
    }
    $scope.$destroy();
-    this.$element.triggerHandler !('$destroy');
+
+    // Clean the jQuery/jqLite data on the component+child elements.
+    // Equivelent to how jQuery/jqLite invoke `cleanData` on an Element (this.element)
+    //  https://github.com/jquery/jquery/blob/e743cbd28553267f955f71ea7248377915613fd9/src/manipulation.js#L223
+    //  https://github.com/angular/angular.js/blob/26ddc5f830f902a3d22f4b2aab70d86d4d688c82/src/jqLite.js#L306-L312
+    // `cleanData` will invoke the AngularJS `$destroy` DOM event
+    //  https://github.com/angular/angular.js/blob/26ddc5f830f902a3d22f4b2aab70d86d4d688c82/src/Angular.js#L1911-L1924
+    angular.element.cleanData([this.element]);
+    angular.element.cleanData(this.element.querySelectorAll('*'));
  }

  prepareTransclusion(): angular.ILinkFn|undefined {
--- a/packages/upgrade/test/dynamic/upgrade_spec.ts
+++ b/packages/upgrade/test/dynamic/upgrade_spec.ts
@ -12,6 +12,7 @@ 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 {$apply, $digest, html, multiTrim, withEachNg1Version} from './test_helpers';

 declare global {
@ -2177,9 +2178,10 @@ withEachNg1Version(() => {
             });
           }));

-        it('should emit `$destroy` on `$element`', fakeAsync(() => {
+        it('should emit `$destroy` on `$element` and descendants', fakeAsync(() => {
             const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
             const elementDestroyListener = jasmine.createSpy('elementDestroyListener');
+             const descendantDestroyListener = jasmine.createSpy('descendantDestroyListener');
             let ng2ComponentInstance: Ng2Component;

             @Component({selector: 'ng2', template: '<div *ngIf="!ng2Destroy"><ng1></ng1></div>'})
@ -2194,9 +2196,13 @@ withEachNg1Version(() => {
             // Mocking animations (via `ngAnimateMock`) avoids the issue.
             angular.module('ng1', ['ngAnimateMock'])
                 .component('ng1', {
-                   controller: function($element: angular.IAugmentedJQuery) {
-                     $element.on !('$destroy', elementDestroyListener);
+                   controller: class {
+                     constructor(private $element: angular.IAugmentedJQuery) {} $onInit() {
+                       this.$element.on !('$destroy', elementDestroyListener);
+                       this.$element.contents !().on !('$destroy', descendantDestroyListener);
+                     }
                   },
+                   template: '<div></div>'
                 })
                 .directive('ng2', adapter.downgradeNg2Component(Ng2Component));

@ -2210,14 +2216,150 @@ withEachNg1Version(() => {
             const element = html('<ng2></ng2>');
             adapter.bootstrap(element, ['ng1']).ready((ref) => {
               const $rootScope = ref.ng1RootScope as any;
+               tick();
+               $rootScope.$digest();

               expect(elementDestroyListener).not.toHaveBeenCalled();
+               expect(descendantDestroyListener).not.toHaveBeenCalled();

               ng2ComponentInstance.ng2Destroy = true;
               tick();
               $rootScope.$digest();

               expect(elementDestroyListener).toHaveBeenCalledTimes(1);
+               expect(descendantDestroyListener).toHaveBeenCalledTimes(1);
+             });
+           }));
+
+        it('should clear data on `$element` and descendants', fakeAsync(() => {
+             const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
+             let ng1ComponentElement: angular.IAugmentedJQuery;
+             let ng2ComponentAInstance: Ng2ComponentA;
+
+             // Define `ng1Component`
+             const ng1Component: angular.IComponent = {
+               controller: class {
+                 constructor(private $element: angular.IAugmentedJQuery) {} $onInit() {
+                   this.$element.data !('test', 1);
+                   this.$element.contents !().data !('test', 2);
+
+                   ng1ComponentElement = this.$element;
+                 }
+               },
+               template: '<div></div>'
+             };
+
+             // Define `Ng2Component`
+             @Component({selector: 'ng2A', template: '<ng2B *ngIf="!destroyIt"></ng2B>'})
+             class Ng2ComponentA {
+               destroyIt = false;
+
+               constructor() { ng2ComponentAInstance = this; }
+             }
+
+             @Component({selector: 'ng2B', template: '<ng1></ng1>'})
+             class Ng2ComponentB {
+             }
+
+             // Define `ng1Module`
+             angular.module('ng1Module', [])
+                 .component('ng1', ng1Component)
+                 .directive('ng2A', adapter.downgradeNg2Component(Ng2ComponentA));
+
+             // Define `Ng2Module`
+             @NgModule({
+               declarations: [adapter.upgradeNg1Component('ng1'), Ng2ComponentA, Ng2ComponentB],
+               entryComponents: [Ng2ComponentA],
+               imports: [BrowserModule]
+             })
+             class Ng2Module {
+               ngDoBootstrap() {}
+             }
+
+             // Bootstrap
+             const element = html(`<ng2-a></ng2-a>`);
+
+             adapter.bootstrap(element, ['ng1Module']).ready((ref) => {
+               const $rootScope = ref.ng1RootScope as any;
+               tick();
+               $rootScope.$digest();
+               expect(ng1ComponentElement.data !('test')).toBe(1);
+               expect(ng1ComponentElement.contents !().data !('test')).toBe(2);
+
+               ng2ComponentAInstance.destroyIt = true;
+               tick();
+               $rootScope.$digest();
+
+               expect(ng1ComponentElement.data !('test')).toBeUndefined();
+               expect(ng1ComponentElement.contents !().data !('test')).toBeUndefined();
+             });
+           }));
+
+        it('should clear dom listeners on `$element` and descendants`', fakeAsync(() => {
+             const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
+             const elementClickListener = jasmine.createSpy('elementClickListener');
+             const descendantClickListener = jasmine.createSpy('descendantClickListener');
+             let ng1DescendantElement: angular.IAugmentedJQuery;
+             let ng2ComponentAInstance: Ng2ComponentA;
+
+             // Define `ng1Component`
+             const ng1Component: angular.IComponent = {
+               controller: class {
+                 constructor(private $element: angular.IAugmentedJQuery) {} $onInit() {
+                   ng1DescendantElement = this.$element.contents !();
+
+                   this.$element.on !('click', elementClickListener);
+                   ng1DescendantElement.on !('click', descendantClickListener);
+                 }
+               },
+               template: '<div></div>'
+             };
+
+             // Define `Ng2Component`
+             @Component({selector: 'ng2A', template: '<ng2B *ngIf="!destroyIt"></ng2B>'})
+             class Ng2ComponentA {
+               destroyIt = false;
+
+               constructor() { ng2ComponentAInstance = this; }
+             }
+
+             @Component({selector: 'ng2B', template: '<ng1></ng1>'})
+             class Ng2ComponentB {
+             }
+
+             // Define `ng1Module`
+             angular.module('ng1Module', [])
+                 .component('ng1', ng1Component)
+                 .directive('ng2A', adapter.downgradeNg2Component(Ng2ComponentA));
+
+             // Define `Ng2Module`
+             @NgModule({
+               declarations: [adapter.upgradeNg1Component('ng1'), Ng2ComponentA, Ng2ComponentB],
+               entryComponents: [Ng2ComponentA],
+               imports: [BrowserModule]
+             })
+             class Ng2Module {
+               ngDoBootstrap() {}
+             }
+
+             // Bootstrap
+             const element = html(`<ng2-a></ng2-a>`);
+
+             adapter.bootstrap(element, ['ng1Module']).ready((ref) => {
+               const $rootScope = ref.ng1RootScope as any;
+               tick();
+               $rootScope.$digest();
+               (ng1DescendantElement[0] as HTMLElement).click();
+               expect(elementClickListener).toHaveBeenCalledTimes(1);
+               expect(descendantClickListener).toHaveBeenCalledTimes(1);
+
+               ng2ComponentAInstance.destroyIt = true;
+               tick();
+               $rootScope.$digest();
+
+               (ng1DescendantElement[0] as HTMLElement).click();
+               expect(elementClickListener).toHaveBeenCalledTimes(1);
+               expect(descendantClickListener).toHaveBeenCalledTimes(1);
             });
           }));
      });
--- a/packages/upgrade/test/static/integration/upgrade_component_spec.ts
+++ b/packages/upgrade/test/static/integration/upgrade_component_spec.ts
@ -3633,8 +3633,9 @@ withEachNg1Version(() => {
           });
         }));

-      it('should emit `$destroy` on `$element`', async(() => {
+      it('should emit `$destroy` on `$element` and descendants', async(() => {
           const elementDestroyListener = jasmine.createSpy('elementDestroyListener');
+           const descendantDestroyListener = jasmine.createSpy('descendantDestroyListener');
           let ng2ComponentAInstance: Ng2ComponentA;

           // Define `ng1Component`
@ -3642,8 +3643,10 @@ withEachNg1Version(() => {
             controller: class {
               constructor($element: angular.IAugmentedJQuery) {
                 $element.on !('$destroy', elementDestroyListener);
+                 $element.contents !().on !('$destroy', descendantDestroyListener);
               }
-             }
+             },
+             template: '<div></div>'
           };

           // Define `Ng1ComponentFacade`
@ -3686,11 +3689,151 @@ withEachNg1Version(() => {

           bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(adapter => {
             expect(elementDestroyListener).not.toHaveBeenCalled();
+             expect(descendantDestroyListener).not.toHaveBeenCalled();

             ng2ComponentAInstance.destroyIt = true;
             $digest(adapter);

             expect(elementDestroyListener).toHaveBeenCalledTimes(1);
+             expect(descendantDestroyListener).toHaveBeenCalledTimes(1);
+           });
+         }));
+
+      it('should clear data on `$element` and descendants`', async(() => {
+           let ng1ComponentElement: angular.IAugmentedJQuery;
+           let ng2ComponentAInstance: Ng2ComponentA;
+
+           // Define `ng1Component`
+           const ng1Component: angular.IComponent = {
+             controller: class {
+               constructor($element: angular.IAugmentedJQuery) {
+                 $element.data !('test', 1);
+                 $element.contents !().data !('test', 2);
+
+                 ng1ComponentElement = $element;
+               }
+             },
+             template: '<div></div>'
+           };
+
+           // Define `Ng1ComponentFacade`
+           @Directive({selector: 'ng1'})
+           class Ng1ComponentFacade extends UpgradeComponent {
+             constructor(elementRef: ElementRef, injector: Injector) {
+               super('ng1', elementRef, injector);
+             }
+           }
+
+           // Define `Ng2Component`
+           @Component({selector: 'ng2A', template: '<ng2B *ngIf="!destroyIt"></ng2B>'})
+           class Ng2ComponentA {
+             destroyIt = false;
+
+             constructor() { ng2ComponentAInstance = this; }
+           }
+
+           @Component({selector: 'ng2B', template: '<ng1></ng1>'})
+           class Ng2ComponentB {
+           }
+
+           // Define `ng1Module`
+           const ng1Module = angular.module('ng1Module', [])
+                                 .component('ng1', ng1Component)
+                                 .directive('ng2A', downgradeComponent({component: Ng2ComponentA}));
+
+           // Define `Ng2Module`
+           @NgModule({
+             declarations: [Ng1ComponentFacade, Ng2ComponentA, Ng2ComponentB],
+             entryComponents: [Ng2ComponentA],
+             imports: [BrowserModule, UpgradeModule]
+           })
+           class Ng2Module {
+             ngDoBootstrap() {}
+           }
+
+           // Bootstrap
+           const element = html(`<ng2-a></ng2-a>`);
+
+           bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(adapter => {
+             expect(ng1ComponentElement.data !('test')).toBe(1);
+             expect(ng1ComponentElement.contents !().data !('test')).toBe(2);
+
+             ng2ComponentAInstance.destroyIt = true;
+             $digest(adapter);
+
+             expect(ng1ComponentElement.data !('test')).toBeUndefined();
+             expect(ng1ComponentElement.contents !().data !('test')).toBeUndefined();
+           });
+         }));
+
+      it('should clear dom listeners on `$element` and descendants`', async(() => {
+           const elementClickListener = jasmine.createSpy('elementClickListener');
+           const descendantClickListener = jasmine.createSpy('descendantClickListener');
+           let ng1DescendantElement: angular.IAugmentedJQuery;
+           let ng2ComponentAInstance: Ng2ComponentA;
+
+           // Define `ng1Component`
+           const ng1Component: angular.IComponent = {
+             controller: class {
+               constructor($element: angular.IAugmentedJQuery) {
+                 ng1DescendantElement = $element.contents !();
+
+                 $element.on !('click', elementClickListener);
+                 ng1DescendantElement.on !('click', descendantClickListener);
+               }
+             },
+             template: '<div></div>'
+           };
+
+           // Define `Ng1ComponentFacade`
+           @Directive({selector: 'ng1'})
+           class Ng1ComponentFacade extends UpgradeComponent {
+             constructor(elementRef: ElementRef, injector: Injector) {
+               super('ng1', elementRef, injector);
+             }
+           }
+
+           // Define `Ng2Component`
+           @Component({selector: 'ng2A', template: '<ng2B *ngIf="!destroyIt"></ng2B>'})
+           class Ng2ComponentA {
+             destroyIt = false;
+
+             constructor() { ng2ComponentAInstance = this; }
+           }
+
+           @Component({selector: 'ng2B', template: '<ng1></ng1>'})
+           class Ng2ComponentB {
+           }
+
+           // Define `ng1Module`
+           const ng1Module = angular.module('ng1Module', [])
+                                 .component('ng1', ng1Component)
+                                 .directive('ng2A', downgradeComponent({component: Ng2ComponentA}));
+
+           // Define `Ng2Module`
+           @NgModule({
+             declarations: [Ng1ComponentFacade, Ng2ComponentA, Ng2ComponentB],
+             entryComponents: [Ng2ComponentA],
+             imports: [BrowserModule, UpgradeModule]
+           })
+           class Ng2Module {
+             ngDoBootstrap() {}
+           }
+
+           // Bootstrap
+           const element = html(`<ng2-a></ng2-a>`);
+
+           bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(adapter => {
+             (ng1DescendantElement[0] as HTMLElement).click();
+             expect(elementClickListener).toHaveBeenCalledTimes(1);
+             expect(descendantClickListener).toHaveBeenCalledTimes(1);
+
+             ng2ComponentAInstance.destroyIt = true;
+             $digest(adapter);
+
+             (ng1DescendantElement[0] as HTMLElement).click();
+             expect(elementClickListener).toHaveBeenCalledTimes(1);
+             expect(descendantClickListener).toHaveBeenCalledTimes(1);
           });
         }));