From 071934e92ab638b7b69374bbe1a19109c0353b97 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Mon, 1 Oct 2018 21:29:38 -0700 Subject: [PATCH] fix(upgrade): properly destroy upgraded component elements and descendants (#26209) Fixes #26208 PR Close #26209 --- packages/upgrade/src/common/angular1.ts | 18 ++- packages/upgrade/src/common/upgrade_helper.ts | 10 +- packages/upgrade/test/dynamic/upgrade_spec.ts | 148 +++++++++++++++++- .../integration/upgrade_component_spec.ts | 147 ++++++++++++++++- 4 files changed, 311 insertions(+), 12 deletions(-) diff --git a/packages/upgrade/src/common/angular1.ts b/packages/upgrade/src/common/angular1.ts index bd7d22e366897..02e3b524216ea 100644 --- a/packages/upgrade/src/common/angular1.ts +++ b/packages/upgrade/src/common/angular1.ts @@ -214,24 +214,29 @@ export interface INgModelController { $name: string; } -function noNg() { +function noNg(): never { throw new Error('AngularJS v1.x is not loaded!'); } +const noNgElement: typeof angular.element = (() => noNg()) as any; +noNgElement.cleanData = noNg; 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 -} = { +} = { bootstrap: noNg, module: noNg, - element: noNg, - version: undefined, + element: noNgElement, + version: undefined as any, resumeBootstrap: noNg, getTestability: noNg }; @@ -281,7 +286,8 @@ 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 = (e => angular.element(e)) as typeof angular.element; +element.cleanData = nodes => angular.element.cleanData(nodes); export const resumeBootstrap: typeof angular.resumeBootstrap = () => angular.resumeBootstrap(); diff --git a/packages/upgrade/src/common/upgrade_helper.ts b/packages/upgrade/src/common/upgrade_helper.ts index 999f83f1d025c..ad18cacf8eb95 100644 --- 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 { diff --git a/packages/upgrade/test/dynamic/upgrade_spec.ts b/packages/upgrade/test/dynamic/upgrade_spec.ts index 2e17c04410a84..2544ed3dec262 100644 --- 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: '
'}) @@ -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: '
' }) .directive('ng2', adapter.downgradeNg2Component(Ng2Component)); @@ -2210,14 +2216,150 @@ withEachNg1Version(() => { const element = html(''); 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: '
' + }; + + // Define `Ng2Component` + @Component({selector: 'ng2A', template: ''}) + class Ng2ComponentA { + destroyIt = false; + + constructor() { ng2ComponentAInstance = this; } + } + + @Component({selector: 'ng2B', template: ''}) + 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(``); + + 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: '
' + }; + + // Define `Ng2Component` + @Component({selector: 'ng2A', template: ''}) + class Ng2ComponentA { + destroyIt = false; + + constructor() { ng2ComponentAInstance = this; } + } + + @Component({selector: 'ng2B', template: ''}) + 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(``); + + 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); }); })); }); diff --git a/packages/upgrade/test/static/integration/upgrade_component_spec.ts b/packages/upgrade/test/static/integration/upgrade_component_spec.ts index 7e4e62347761b..fe20a9b00f2cd 100644 --- 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: '
' }; // 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: '
' + }; + + // Define `Ng1ComponentFacade` + @Directive({selector: 'ng1'}) + class Ng1ComponentFacade extends UpgradeComponent { + constructor(elementRef: ElementRef, injector: Injector) { + super('ng1', elementRef, injector); + } + } + + // Define `Ng2Component` + @Component({selector: 'ng2A', template: ''}) + class Ng2ComponentA { + destroyIt = false; + + constructor() { ng2ComponentAInstance = this; } + } + + @Component({selector: 'ng2B', template: ''}) + 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(``); + + 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: '
' + }; + + // Define `Ng1ComponentFacade` + @Directive({selector: 'ng1'}) + class Ng1ComponentFacade extends UpgradeComponent { + constructor(elementRef: ElementRef, injector: Injector) { + super('ng1', elementRef, injector); + } + } + + // Define `Ng2Component` + @Component({selector: 'ng2A', template: ''}) + class Ng2ComponentA { + destroyIt = false; + + constructor() { ng2ComponentAInstance = this; } + } + + @Component({selector: 'ng2B', template: ''}) + 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(``); + + 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); }); }));