diff --git a/packages/upgrade/src/common/src/angular1.ts b/packages/upgrade/src/common/src/angular1.ts index b62bf6b71832..b4a8b94d1286 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.ts b/packages/upgrade/src/common/src/downgrade_component.ts index bb2f00fce0b4..b0949e3d8583 100644 --- a/packages/upgrade/src/common/src/downgrade_component.ts +++ b/packages/upgrade/src/common/src/downgrade_component.ts @@ -174,8 +174,8 @@ export function downgradeComponent(info: { const injectorPromise = new ParentInjectorPromise(element); const facade = new DowngradeComponentAdapter( - element, attrs, scope, ngModel, injector, $injector, $compile, $parse, - componentFactory, wrapCallback); + element, attrs, scope, ngModel, injector, $compile, $parse, componentFactory, + wrapCallback); const projectableNodes = facade.compileContents(); facade.createComponent(projectableNodes); diff --git a/packages/upgrade/src/common/src/downgrade_component_adapter.ts b/packages/upgrade/src/common/src/downgrade_component_adapter.ts index 152df408eb44..5022a58aaca2 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, IInjectorService, 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'; @@ -33,8 +33,8 @@ export class DowngradeComponentAdapter { constructor( private element: IAugmentedJQuery, private attrs: IAttributes, private scope: IScope, private ngModel: INgModelController, private parentInjector: Injector, - private $injector: IInjectorService, private $compile: ICompileService, - private $parse: IParseService, private componentFactory: ComponentFactory, + private $compile: ICompileService, private $parse: IParseService, + private componentFactory: ComponentFactory, private wrapCallback: (cb: () => T) => () => T) { this.componentScope = scope.$new(); } @@ -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(); } }); @@ -250,7 +277,6 @@ export class DowngradeComponentAdapter { */ export function groupNodesBySelector(ngContentSelectors: string[], nodes: Node[]): Node[][] { const projectableNodes: Node[][] = []; - let wildcardNgContentIndex: number; for (let i = 0, ii = ngContentSelectors.length; i < ii; ++i) { projectableNodes[i] = []; diff --git a/packages/upgrade/src/common/src/upgrade_helper.ts b/packages/upgrade/src/common/src/upgrade_helper.ts index f5414cf8eced..0d520eb9e6a4 100644 --- a/packages/upgrade/src/common/src/upgrade_helper.ts +++ b/packages/upgrade/src/common/src/upgrade_helper.ts @@ -41,8 +41,7 @@ export class UpgradeHelper { private readonly $controller: IControllerService; constructor( - private injector: Injector, private name: string, elementRef: ElementRef, - directive?: IDirective) { + injector: Injector, private name: string, elementRef: ElementRef, directive?: IDirective) { this.$injector = injector.get($INJECTOR); this.$compile = this.$injector.get($COMPILE); this.$controller = this.$injector.get($CONTROLLER); diff --git a/packages/upgrade/src/common/test/downgrade_component_adapter_spec.ts b/packages/upgrade/src/common/test/downgrade_component_adapter_spec.ts index e59470ca88c1..0a2300a2e79d 100644 --- a/packages/upgrade/src/common/test/downgrade_component_adapter_spec.ts +++ b/packages/upgrade/src/common/test/downgrade_component_adapter_spec.ts @@ -131,7 +131,6 @@ withEachNg1Version(() => { let scope: angular.IScope; // mock let ngModel = undefined as any; let parentInjector: Injector; // testbed - let $injector = undefined as any; let $compile = undefined as any; let $parse = undefined as any; let componentFactory: ComponentFactory; // testbed @@ -166,8 +165,8 @@ withEachNg1Version(() => { parentInjector = TestBed; return new DowngradeComponentAdapter( - element, attrs, scope, ngModel, parentInjector, $injector, $compile, $parse, - componentFactory, wrapCallback); + element, attrs, scope, ngModel, parentInjector, $compile, $parse, componentFactory, + wrapCallback); } beforeEach(() => { diff --git a/packages/upgrade/src/dynamic/test/upgrade_spec.ts b/packages/upgrade/src/dynamic/test/upgrade_spec.ts index 9ab7ba7ff807..3d33cc59be02 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: '
  • test1
  • test2
'}) 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 e5ad0978b4f4..ec88d423d158 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: '
  • test1
  • test2
'}) 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 020081077fea..cc34ec57b7f5 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: '
  • test1
  • test2
'}) + 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;