Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10bf47e
fix(upgrade): properly destroy upgraded component elements and descen…
jbedard Oct 2, 2018
1d917b0
fixup! fix(upgrade): properly destroy upgraded component elements and…
jbedard Oct 8, 2018
f5447a3
fixup! fix(upgrade): properly destroy upgraded component elements and…
gkalpak Oct 9, 2018
3586fc6
fixup! fix(upgrade): properly destroy upgraded component elements and…
gkalpak Oct 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions packages/upgrade/src/common/angular1.ts
View file
Open in desktop
Expand Up @@ -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
} = <any>{
} = {
bootstrap: noNg,
module: noNg,
element: noNg,
version: undefined,
element: noNgElement,
version: undefined as any,
resumeBootstrap: noNg,
getTestability: noNg
};
Expand Down Expand Up @@ -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();

Expand Down
10 changes: 9 additions & 1 deletion packages/upgrade/src/common/upgrade_helper.ts
View file
Open in desktop
Expand Up @@ -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
jbedard marked this conversation as resolved.
Show resolved Hide resolved
// `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 {
Expand Down
148 changes: 145 additions & 3 deletions packages/upgrade/test/dynamic/upgrade_spec.ts
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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>'})
Expand All @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, clang-format 👌
🤢 🤢 🤢

this.$element.on !('$destroy', elementDestroyListener);
this.$element.contents !().on !('$destroy', descendantDestroyListener);
}
},
template: '<div></div>'
})
.directive('ng2', adapter.downgradeNg2Component(Ng2Component));

Expand All @@ -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);
});
}));
});
Expand Down