From 9623e7c639bd30bb03869b9e62a64cf86969ce3e Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Mon, 8 Oct 2018 14:34:47 -0700 Subject: [PATCH] Revert "fix(upgrade): properly destroy upgraded component elements and descendants (#26209)" This reverts commit 5a31bde6496c63d82d86227d7772c2127c28b08c. --- packages/upgrade/src/common/angular1.ts | 11 +- 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, 9 insertions(+), 307 deletions(-) diff --git a/packages/upgrade/src/common/angular1.ts b/packages/upgrade/src/common/angular1.ts index d11542889e..bd7d22e366 100644 --- a/packages/upgrade/src/common/angular1.ts +++ b/packages/upgrade/src/common/angular1.ts @@ -223,17 +223,14 @@ let angular: { bootstrap: (e: Element, modules: (string | IInjectable)[], config?: IAngularBootstrapConfig) => IInjectorService, module: (prefix: string, dependencies?: string[]) => IModule, - element: { - (e: string | Element | Document | IAugmentedJQuery): IAugmentedJQuery; - cleanData: (nodes: Node[] | NodeList) => void; - }, + element: (e: string | Element | Document | IAugmentedJQuery) => IAugmentedJQuery, version: {major: number}, resumeBootstrap: () => void, getTestability: (e: Element) => ITestabilityService } = { bootstrap: noNg, module: noNg, - element: Object.assign(() => noNg(), {cleanData: noNg}), + element: noNg, version: undefined, resumeBootstrap: noNg, getTestability: noNg @@ -284,9 +281,7 @@ 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 = Object.assign( - (e: string | Element | Document | IAugmentedJQuery) => angular.element(e), - {cleanData: (nodes: Node[] | NodeList) => angular.element.cleanData(nodes)}); +export const element: typeof angular.element = e => angular.element(e); 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 ad18cacf8e..999f83f1d0 100644 --- a/packages/upgrade/src/common/upgrade_helper.ts +++ b/packages/upgrade/src/common/upgrade_helper.ts @@ -124,15 +124,7 @@ export class UpgradeHelper { controllerInstance.$onDestroy(); } $scope.$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('*')); + this.$element.triggerHandler !('$destroy'); } prepareTransclusion(): angular.ILinkFn|undefined { diff --git a/packages/upgrade/test/dynamic/upgrade_spec.ts b/packages/upgrade/test/dynamic/upgrade_spec.ts index 2544ed3dec..2e17c04410 100644 --- a/packages/upgrade/test/dynamic/upgrade_spec.ts +++ b/packages/upgrade/test/dynamic/upgrade_spec.ts @@ -12,7 +12,6 @@ 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 { @@ -2178,10 +2177,9 @@ withEachNg1Version(() => { }); })); - it('should emit `$destroy` on `$element` and descendants', fakeAsync(() => { + it('should emit `$destroy` on `$element`', 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: '
'}) @@ -2196,13 +2194,9 @@ withEachNg1Version(() => { // Mocking animations (via `ngAnimateMock`) avoids the issue. angular.module('ng1', ['ngAnimateMock']) .component('ng1', { - controller: class { - constructor(private $element: angular.IAugmentedJQuery) {} $onInit() { - this.$element.on !('$destroy', elementDestroyListener); - this.$element.contents !().on !('$destroy', descendantDestroyListener); - } + controller: function($element: angular.IAugmentedJQuery) { + $element.on !('$destroy', elementDestroyListener); }, - template: '
' }) .directive('ng2', adapter.downgradeNg2Component(Ng2Component)); @@ -2216,150 +2210,14 @@ 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 fe20a9b00f..7e4e623477 100644 --- a/packages/upgrade/test/static/integration/upgrade_component_spec.ts +++ b/packages/upgrade/test/static/integration/upgrade_component_spec.ts @@ -3633,9 +3633,8 @@ withEachNg1Version(() => { }); })); - it('should emit `$destroy` on `$element` and descendants', async(() => { + it('should emit `$destroy` on `$element`', async(() => { const elementDestroyListener = jasmine.createSpy('elementDestroyListener'); - const descendantDestroyListener = jasmine.createSpy('descendantDestroyListener'); let ng2ComponentAInstance: Ng2ComponentA; // Define `ng1Component` @@ -3643,10 +3642,8 @@ withEachNg1Version(() => { controller: class { constructor($element: angular.IAugmentedJQuery) { $element.on !('$destroy', elementDestroyListener); - $element.contents !().on !('$destroy', descendantDestroyListener); } - }, - template: '
' + } }; // Define `Ng1ComponentFacade` @@ -3689,151 +3686,11 @@ 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); }); }));