diff --git a/packages/upgrade/src/common/src/angular1.ts b/packages/upgrade/src/common/src/angular1.ts index b62bf6b718..b4a8b94d12 100644 --- a/packages/upgrade/src/common/src/angular1.ts +++ b/packages/upgrade/src/common/src/angular1.ts @@ -126,6 +126,7 @@ export type IAugmentedJQuery = Node[]&{ data?: (name: string, value?: any) => any; text?: () => string; inheritedData?: (name: string, value?: any) => any; + children?: () => IAugmentedJQuery; contents?: () => IAugmentedJQuery; parent?: () => IAugmentedJQuery; empty?: () => void; diff --git a/packages/upgrade/src/common/src/downgrade_component_adapter.ts b/packages/upgrade/src/common/src/downgrade_component_adapter.ts index 91b84ab676..5022a58aac 100644 --- a/packages/upgrade/src/common/src/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/src/downgrade_component_adapter.ts @@ -8,7 +8,7 @@ import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentRef, EventEmitter, Injector, OnChanges, SimpleChange, SimpleChanges, StaticProvider, Testability, TestabilityRegistry, Type} from '@angular/core'; -import {IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1'; +import {element as angularElement, IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1'; import {PropertyBinding} from './component_info'; import {$SCOPE} from './constants'; import {getTypeName, hookupNgModel, strictEquals} from './util'; @@ -216,11 +216,38 @@ export class DowngradeComponentAdapter { const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); let destroyed = false; - this.element.on!('$destroy', () => this.componentScope.$destroy()); + this.element.on!('$destroy', () => { + // The `$destroy` event may have been triggered by the `cleanData()` call in the + // `componentScope` `$destroy` handler below. In that case, we don't want to call + // `componentScope.$destroy()` again. + if (!destroyed) this.componentScope.$destroy(); + }); this.componentScope.$on('$destroy', () => { if (!destroyed) { destroyed = true; testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement); + + // The `componentScope` might be getting destroyed, because an ancestor element is being + // removed/destroyed. If that is the case, jqLite/jQuery would normally invoke `cleanData()` + // on the removed element and all descendants. + // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355 + // https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182 + // + // Here, however, `destroyComponentRef()` may under some circumstances remove the element + // from the DOM and therefore it will no longer be a descendant of the removed element when + // `cleanData()` is called. This would result in a memory leak, because the element's data + // and event handlers (and all objects directly or indirectly referenced by them) would be + // retained. + // + // To ensure the element is always properly cleaned up, we manually call `cleanData()` on + // this element and its descendants before destroying the `ComponentRef`. + // + // NOTE: + // `cleanData()` also will invoke the AngularJS `$destroy` event on the element: + // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945 + angularElement.cleanData(this.element); + angularElement.cleanData((this.element[0] as Element).querySelectorAll('*')); + destroyComponentRef(); } }); diff --git a/packages/upgrade/src/dynamic/test/upgrade_spec.ts b/packages/upgrade/src/dynamic/test/upgrade_spec.ts index 9ab7ba7ff8..3d33cc59be 100644 --- a/packages/upgrade/src/dynamic/test/upgrade_spec.ts +++ b/packages/upgrade/src/dynamic/test/upgrade_spec.ts @@ -665,23 +665,16 @@ withEachNg1Version(() => { it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => { const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); const ng1Module = angular.module_('ng1', []); - const onDestroyed: EventEmitter = new EventEmitter(); + let ng2ComponentDestroyed = false; - ng1Module.directive('ng1', () => { - return { - template: '
', - controller: function($rootScope: any, $timeout: Function) { - $timeout(() => { - $rootScope.destroyIt = true; - }); - } - }; - }); + ng1Module.directive('ng1', () => ({ + template: '
', + })); - @Component({selector: 'ng2', template: 'test'}) + @Component({selector: 'ng2', template: ''}) class Ng2 { ngOnDestroy() { - onDestroyed.emit('destroyed'); + ng2ComponentDestroyed = true; } } @@ -695,9 +688,35 @@ withEachNg1Version(() => { ng1Module.directive('ng2', adapter.downgradeNg2Component(Ng2)); const element = html(''); adapter.bootstrap(element, ['ng1']).ready((ref) => { - onDestroyed.subscribe(() => { - ref.dispose(); - }); + const ng2Element = angular.element(element.querySelector('ng2') as Element); + const ng2Descendants = + Array.from(element.querySelectorAll('ng2 li')).map(angular.element); + let ng2ElementDestroyed = false; + let ng2DescendantsDestroyed = ng2Descendants.map(() => false); + + ng2Element.data!('test', 42); + ng2Descendants.forEach((elem, i) => elem.data!('test', i)); + ng2Element.on!('$destroy', () => ng2ElementDestroyed = true); + ng2Descendants.forEach( + (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true)); + + expect(element.textContent).toBe('test1test2'); + expect(ng2Element.data!('test')).toBe(42); + ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i)); + expect(ng2ElementDestroyed).toBe(false); + expect(ng2DescendantsDestroyed).toEqual([false, false]); + expect(ng2ComponentDestroyed).toBe(false); + + ref.ng1RootScope.$apply('destroyIt = true'); + + expect(element.textContent).toBe(''); + expect(ng2Element.data!('test')).toBeUndefined(); + ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined()); + expect(ng2ElementDestroyed).toBe(true); + expect(ng2DescendantsDestroyed).toEqual([true, true]); + expect(ng2ComponentDestroyed).toBe(true); + + ref.dispose(); }); })); diff --git a/packages/upgrade/static/test/integration/downgrade_component_spec.ts b/packages/upgrade/static/test/integration/downgrade_component_spec.ts index e5ad0978b4..ec88d423d1 100644 --- a/packages/upgrade/static/test/integration/downgrade_component_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_component_spec.ts @@ -536,7 +536,7 @@ withEachNg1Version(() => { it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => { let destroyed = false; - @Component({selector: 'ng2', template: 'test'}) + @Component({selector: 'ng2', template: ''}) class Ng2Component implements OnDestroy { ngOnDestroy() { destroyed = true; @@ -563,14 +563,35 @@ withEachNg1Version(() => { platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => { const adapter = ref.injector.get(UpgradeModule) as UpgradeModule; adapter.bootstrap(element, [ng1Module.name]); - expect(element.textContent).toContain('test'); + + const ng2Element = angular.element(element.querySelector('ng2') as Element); + const ng2Descendants = + Array.from(element.querySelectorAll('ng2 li')).map(angular.element); + let ng2ElementDestroyed = false; + let ng2DescendantsDestroyed = [false, false]; + + ng2Element.data!('test', 42); + ng2Descendants.forEach((elem, i) => elem.data!('test', i)); + ng2Element.on!('$destroy', () => ng2ElementDestroyed = true); + ng2Descendants.forEach( + (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true)); + + expect(element.textContent).toBe('test1test2'); expect(destroyed).toBe(false); + expect(ng2Element.data!('test')).toBe(42); + ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i)); + expect(ng2ElementDestroyed).toBe(false); + expect(ng2DescendantsDestroyed).toEqual([false, false]); const $rootScope = adapter.$injector.get('$rootScope'); $rootScope.$apply('destroyIt = true'); - expect(element.textContent).not.toContain('test'); + expect(element.textContent).toBe(''); expect(destroyed).toBe(true); + expect(ng2Element.data!('test')).toBeUndefined(); + ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined()); + expect(ng2ElementDestroyed).toBe(true); + expect(ng2DescendantsDestroyed).toEqual([true, true]); }); })); diff --git a/packages/upgrade/static/test/integration/downgrade_module_spec.ts b/packages/upgrade/static/test/integration/downgrade_module_spec.ts index 020081077f..cc34ec57b7 100644 --- a/packages/upgrade/static/test/integration/downgrade_module_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_module_spec.ts @@ -1187,6 +1187,69 @@ withEachNg1Version(() => { }); })); + it('should properly run cleanup when a downgraded component is destroyed', + waitForAsync(() => { + let destroyed = false; + + @Component({selector: 'ng2', template: ''}) + class Ng2Component implements OnDestroy { + ngOnDestroy() { + destroyed = true; + } + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module_('ng1', [lazyModuleName]) + .directive( + 'ng2', downgradeComponent({component: Ng2Component, propagateDigest})); + + const element = html('
'); + const $injector = angular.bootstrap(element, [ng1Module.name]); + const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService; + + setTimeout(() => { // Wait for the module to be bootstrapped. + const ng2Element = angular.element(element.querySelector('ng2') as Element); + const ng2Descendants = + Array.from(element.querySelectorAll('ng2 li')).map(angular.element); + let ng2ElementDestroyed = false; + let ng2DescendantsDestroyed = [false, false]; + + ng2Element.data!('test', 42); + ng2Descendants.forEach((elem, i) => elem.data!('test', i)); + ng2Element.on!('$destroy', () => ng2ElementDestroyed = true); + ng2Descendants.forEach( + (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true)); + + expect(element.textContent).toBe('test1test2'); + expect(destroyed).toBe(false); + expect(ng2Element.data!('test')).toBe(42); + ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i)); + expect(ng2ElementDestroyed).toBe(false); + expect(ng2DescendantsDestroyed).toEqual([false, false]); + + $rootScope.$apply('hideNg2 = true'); + + expect(element.textContent).toBe(''); + expect(destroyed).toBe(true); + expect(ng2Element.data!('test')).toBeUndefined(); + ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined()); + expect(ng2ElementDestroyed).toBe(true); + expect(ng2DescendantsDestroyed).toEqual([true, true]); + }); + })); + it('should only retrieve the Angular zone once (and cache it for later use)', fakeAsync(() => { let count = 0;