Skip to content

Commit 7d18ca3

Browse files
gkalpakgkalpak
committed
fixup! fix(upgrade): avoid memory leak when removing downgraded components
1 parent b3f23b4 commit 7d18ca3

File tree

4 files changed

+62
-58
lines changed

4 files changed

+62
-58
lines changed

‎packages/upgrade/src/common/src/downgrade_component_adapter.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ export class DowngradeComponentAdapter {
246246
// `cleanData()` also will invoke the AngularJS `$destroy` event on the element:
247247
// https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945
248248
angularElement.cleanData(this.element);
249-
angularElement.cleanData(this.element.children!());
249+
angularElement.cleanData((this.element[0] as Element).querySelectorAll('*'));
250250

251251
destroyComponentRef();
252252
}

‎packages/upgrade/src/dynamic/test/upgrade_spec.ts‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -668,10 +668,10 @@ withEachNg1Version(() => {
668668
let ng2ComponentDestroyed = false;
669669

670670
ng1Module.directive('ng1', () => ({
671-
template: '<div ng-if="!destroyIt"><ng2></ng2></div>',
672-
}));
671+
template: '<div ng-if="!destroyIt"><ng2></ng2></div>',
672+
}));
673673

674-
@Component({selector: 'ng2', template: '<span>test</span>'})
674+
@Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
675675
class Ng2 {
676676
ngOnDestroy() {
677677
ng2ComponentDestroyed = true;
@@ -689,29 +689,31 @@ withEachNg1Version(() => {
689689
const element = html('<ng1></ng1>');
690690
adapter.bootstrap(element, ['ng1']).ready((ref) => {
691691
const ng2Element = angular.element(element.querySelector('ng2') as Element);
692-
const ng2Children = (ng2Element as any).children!();
692+
const ng2Descendants =
693+
Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
693694
let ng2ElementDestroyed = false;
694-
let ng2ChildrenDestroyed = false;
695+
let ng2DescendantsDestroyed = ng2Descendants.map(() => false);
695696

696-
ng2Element.data!('foo', 1);
697-
ng2Children.data!('bar', 2);
697+
ng2Element.data!('test', 42);
698+
ng2Descendants.forEach((elem, i) => elem.data!('test', i));
698699
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
699-
ng2Children.on!('$destroy', () => ng2ChildrenDestroyed = true);
700+
ng2Descendants.forEach(
701+
(elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
700702

701-
expect(element.textContent).toContain('test');
702-
expect(ng2Element.data!('foo')).toBe(1);
703-
expect(ng2Children.data!('bar')).toBe(2);
703+
expect(element.textContent).toBe('test1test2');
704+
expect(ng2Element.data!('test')).toBe(42);
705+
ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
704706
expect(ng2ElementDestroyed).toBe(false);
705-
expect(ng2ChildrenDestroyed).toBe(false);
707+
expect(ng2DescendantsDestroyed).toEqual([false, false]);
706708
expect(ng2ComponentDestroyed).toBe(false);
707709

708710
ref.ng1RootScope.$apply('destroyIt = true');
709711

710-
expect(element.textContent).not.toContain('test');
711-
expect(ng2Element.data!('foo')).toBeUndefined();
712-
expect(ng2Children.data!('baz')).toBeUndefined();
712+
expect(element.textContent).toBe('');
713+
expect(ng2Element.data!('test')).toBeUndefined();
714+
ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
713715
expect(ng2ElementDestroyed).toBe(true);
714-
expect(ng2ChildrenDestroyed).toBe(true);
716+
expect(ng2DescendantsDestroyed).toEqual([true, true]);
715717
expect(ng2ComponentDestroyed).toBe(true);
716718

717719
ref.dispose();

‎packages/upgrade/static/test/integration/downgrade_component_spec.ts‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -536,7 +536,7 @@ withEachNg1Version(() => {
536536

537537
it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => {
538538
let destroyed = false;
539-
@Component({selector: 'ng2', template: '<span>test</span>'})
539+
@Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
540540
class Ng2Component implements OnDestroy {
541541
ngOnDestroy() {
542542
destroyed = true;
@@ -565,31 +565,33 @@ withEachNg1Version(() => {
565565
adapter.bootstrap(element, [ng1Module.name]);
566566

567567
const ng2Element = angular.element(element.querySelector('ng2') as Element);
568-
const ng2Children = (ng2Element as any).children!();
568+
const ng2Descendants =
569+
Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
569570
let ng2ElementDestroyed = false;
570-
let ng2ChildrenDestroyed = false;
571+
let ng2DescendantsDestroyed = [false, false];
571572

572-
ng2Element.data!('foo', 1);
573-
ng2Children.data!('bar', 2);
573+
ng2Element.data!('test', 42);
574+
ng2Descendants.forEach((elem, i) => elem.data!('test', i));
574575
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
575-
ng2Children.on!('$destroy', () => ng2ChildrenDestroyed = true);
576+
ng2Descendants.forEach(
577+
(elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
576578

577-
expect(element.textContent).toContain('test');
579+
expect(element.textContent).toBe('test1test2');
578580
expect(destroyed).toBe(false);
579-
expect(ng2Element.data!('foo')).toBe(1);
580-
expect(ng2Children.data!('bar')).toBe(2);
581+
expect(ng2Element.data!('test')).toBe(42);
582+
ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
581583
expect(ng2ElementDestroyed).toBe(false);
582-
expect(ng2ChildrenDestroyed).toBe(false);
584+
expect(ng2DescendantsDestroyed).toEqual([false, false]);
583585

584586
const $rootScope = adapter.$injector.get('$rootScope');
585587
$rootScope.$apply('destroyIt = true');
586588

587-
expect(element.textContent).not.toContain('test');
589+
expect(element.textContent).toBe('');
588590
expect(destroyed).toBe(true);
589-
expect(ng2Element.data!('foo')).toBeUndefined();
590-
expect(ng2Children.data!('baz')).toBeUndefined();
591+
expect(ng2Element.data!('test')).toBeUndefined();
592+
ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
591593
expect(ng2ElementDestroyed).toBe(true);
592-
expect(ng2ChildrenDestroyed).toBe(true);
594+
expect(ng2DescendantsDestroyed).toEqual([true, true]);
593595
});
594596
}));
595597

‎packages/upgrade/static/test/integration/downgrade_module_spec.ts‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,7 +1191,7 @@ withEachNg1Version(() => {
11911191
waitForAsync(() => {
11921192
let destroyed = false;
11931193

1194-
@Component({selector: 'ng2', template: '<span>test</span>'})
1194+
@Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
11951195
class Ng2Component implements OnDestroy {
11961196
ngOnDestroy() {
11971197
destroyed = true;
@@ -1219,34 +1219,34 @@ withEachNg1Version(() => {
12191219
const $injector = angular.bootstrap(element, [ng1Module.name]);
12201220
const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService;
12211221

1222-
setTimeout(() => { // Wait for the module to be bootstrapped.
1223-
setTimeout(() => { // Wait for the hostView to be attached (during the `$digest`).
1224-
const ng2Element = angular.element(element.querySelector('ng2') as Element);
1225-
const ng2Children = (ng2Element as any).children!();
1226-
let ng2ElementDestroyed = false;
1227-
let ng2ChildrenDestroyed = false;
1228-
1229-
ng2Element.data!('foo', 1);
1230-
ng2Children.data!('bar', 2);
1231-
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
1232-
ng2Children.on!('$destroy', () => ng2ChildrenDestroyed = true);
1233-
1234-
expect(element.textContent).toContain('test');
1235-
expect(destroyed).toBe(false);
1236-
expect(ng2Element.data!('foo')).toBe(1);
1237-
expect(ng2Children.data!('bar')).toBe(2);
1238-
expect(ng2ElementDestroyed).toBe(false);
1239-
expect(ng2ChildrenDestroyed).toBe(false);
1222+
setTimeout(() => { // Wait for the module to be bootstrapped.
1223+
const ng2Element = angular.element(element.querySelector('ng2') as Element);
1224+
const ng2Descendants =
1225+
Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
1226+
let ng2ElementDestroyed = false;
1227+
let ng2DescendantsDestroyed = [false, false];
1228+
1229+
ng2Element.data!('test', 42);
1230+
ng2Descendants.forEach((elem, i) => elem.data!('test', i));
1231+
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
1232+
ng2Descendants.forEach(
1233+
(elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
1234+
1235+
expect(element.textContent).toBe('test1test2');
1236+
expect(destroyed).toBe(false);
1237+
expect(ng2Element.data!('test')).toBe(42);
1238+
ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
1239+
expect(ng2ElementDestroyed).toBe(false);
1240+
expect(ng2DescendantsDestroyed).toEqual([false, false]);
12401241

1241-
$rootScope.$apply('hideNg2 = true');
1242+
$rootScope.$apply('hideNg2 = true');
12421243

1243-
expect(element.textContent).not.toContain('test');
1244-
expect(destroyed).toBe(true);
1245-
expect(ng2Element.data!('foo')).toBeUndefined();
1246-
expect(ng2Children.data!('baz')).toBeUndefined();
1247-
expect(ng2ElementDestroyed).toBe(true);
1248-
expect(ng2ChildrenDestroyed).toBe(true);
1249-
});
1244+
expect(element.textContent).toBe('');
1245+
expect(destroyed).toBe(true);
1246+
expect(ng2Element.data!('test')).toBeUndefined();
1247+
ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
1248+
expect(ng2ElementDestroyed).toBe(true);
1249+
expect(ng2DescendantsDestroyed).toEqual([true, true]);
12501250
});
12511251
}));
12521252

0 commit comments

Comments
 (0)