Skip to content

Commit 8a85888

Browse files
gkalpakalexeagle
gkalpak
authored and
alexeagle
committed
fix(upgrade): correctly destroy nested downgraded component (#22400)
Previously, when a downgraded component was destroyed in a way that did not trigger the `$destroy` event on the element (e.g. when a parent element was removed from the DOM by Angular, not AngularJS), the `ComponentRef` was not destroyed and unregistered. This commit fixes it by listening for the `$destroy` event on both the element and the scope. Fixes #22392 PR Close #22400
1 parent 83b32a0 commit 8a85888

File tree

6 files changed

+122
-11
lines changed

6 files changed

+122
-11
lines changed

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

+10-6
Original file line numberDiff line numberDiff line change
@@ -209,12 +209,16 @@ export class DowngradeComponentAdapter {
209209

210210
registerCleanup() {
211211
const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy());
212-
213-
this.element.on !('$destroy', () => {
214-
this.componentScope.$destroy();
215-
this.componentRef.injector.get(TestabilityRegistry)
216-
.unregisterApplication(this.componentRef.location.nativeElement);
217-
destroyComponentRef();
212+
let destroyed = false;
213+
214+
this.element.on !('$destroy', () => this.componentScope.$destroy());
215+
this.componentScope.$on('$destroy', () => {
216+
if (!destroyed) {
217+
destroyed = true;
218+
this.componentRef.injector.get(TestabilityRegistry)
219+
.unregisterApplication(this.componentRef.location.nativeElement);
220+
destroyComponentRef();
221+
}
218222
});
219223
}
220224

‎packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts

+2
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,8 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck {
263263
if (this.controllerInstance && isFunction(this.controllerInstance.$onDestroy)) {
264264
this.controllerInstance.$onDestroy();
265265
}
266+
267+
this.componentScope.$destroy();
266268
}
267269

268270
setComponentProperty(name: string, value: any) {

‎packages/upgrade/test/common/downgrade_component_adapter_spec.ts

+7-1
Original file line numberDiff line numberDiff line change
@@ -85,15 +85,21 @@ withEachNg1Version(() => {
8585
let element: angular.IAugmentedJQuery;
8686

8787
class mockScope implements angular.IScope {
88+
private destroyListeners: (() => void)[] = [];
89+
8890
$new() { return this; }
8991
$watch(exp: angular.Ng1Expression, fn?: (a1?: any, a2?: any) => void) {
9092
return () => {};
9193
}
9294
$on(event: string, fn?: (event?: any, ...args: any[]) => void) {
95+
if (event === '$destroy' && fn) {
96+
this.destroyListeners.push(fn);
97+
}
9398
return () => {};
9499
}
95100
$destroy() {
96-
return () => {};
101+
let listener: (() => void)|undefined;
102+
while ((listener = this.destroyListeners.shift())) listener();
97103
}
98104
$apply(exp?: angular.Ng1Expression) {
99105
return () => {};

‎packages/upgrade/test/dynamic/test_helpers.ts

+5
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ import {$ROOT_SCOPE} from '@angular/upgrade/src/common/constants';
1212

1313
export * from '../common/test_helpers';
1414

15+
export function $apply(adapter: UpgradeAdapterRef, exp: angular.Ng1Expression) {
16+
const $rootScope = adapter.ng1Injector.get($ROOT_SCOPE) as angular.IRootScopeService;
17+
$rootScope.$apply(exp);
18+
}
19+
1520
export function $digest(adapter: UpgradeAdapterRef) {
1621
const $rootScope = adapter.ng1Injector.get($ROOT_SCOPE) as angular.IRootScopeService;
1722
$rootScope.$digest();

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

+45-2
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {ChangeDetectorRef, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgModuleFactory, NgZone, OnChanges, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core';
9+
import {ChangeDetectorRef, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgModuleFactory, NgZone, OnChanges, OnDestroy, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core';
1010
import {async, fakeAsync, flushMicrotasks, tick} from '@angular/core/testing';
1111
import {BrowserModule} from '@angular/platform-browser';
1212
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
1313
import * as angular from '@angular/upgrade/src/common/angular1';
1414
import {UpgradeAdapter, UpgradeAdapterRef} from '@angular/upgrade/src/dynamic/upgrade_adapter';
15-
import {$digest, html, multiTrim, withEachNg1Version} from './test_helpers';
15+
import {$apply, $digest, html, multiTrim, withEachNg1Version} from './test_helpers';
1616

1717
declare global {
1818
export var inject: Function;
@@ -582,6 +582,49 @@ withEachNg1Version(() => {
582582
});
583583
}));
584584

585+
it('should properly run cleanup with multiple levels of nesting', async(() => {
586+
const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
587+
let destroyed = false;
588+
589+
@Component(
590+
{selector: 'ng2-outer', template: '<div *ngIf="!destroyIt"><ng1></ng1></div>'})
591+
class Ng2OuterComponent {
592+
@Input() destroyIt = false;
593+
}
594+
595+
@Component({selector: 'ng2-inner', template: 'test'})
596+
class Ng2InnerComponent implements OnDestroy {
597+
ngOnDestroy() { destroyed = true; }
598+
}
599+
600+
@NgModule({
601+
imports: [BrowserModule],
602+
declarations:
603+
[Ng2InnerComponent, Ng2OuterComponent, adapter.upgradeNg1Component('ng1')],
604+
schemas: [NO_ERRORS_SCHEMA],
605+
})
606+
class Ng2Module {
607+
}
608+
609+
const ng1Module =
610+
angular.module('ng1', [])
611+
.directive('ng1', () => ({template: '<ng2-inner></ng2-inner>'}))
612+
.directive('ng2Inner', adapter.downgradeNg2Component(Ng2InnerComponent))
613+
.directive('ng2Outer', adapter.downgradeNg2Component(Ng2OuterComponent));
614+
615+
const element = html('<ng2-outer [destroy-it]="destroyIt"></ng2-outer>');
616+
617+
adapter.bootstrap(element, [ng1Module.name]).ready(ref => {
618+
expect(element.textContent).toBe('test');
619+
expect(destroyed).toBe(false);
620+
621+
$apply(ref, 'destroyIt = true');
622+
623+
expect(element.textContent).toBe('');
624+
expect(destroyed).toBe(true);
625+
});
626+
}));
627+
585628
it('should fallback to the root ng2.injector when compiled outside the dom', async(() => {
586629
const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
587630
const ng1Module = angular.module('ng1', []);

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

+53-2
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core';
9+
import {ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, Directive, ElementRef, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core';
1010
import {async, fakeAsync, tick} from '@angular/core/testing';
1111
import {BrowserModule} from '@angular/platform-browser';
1212
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
13-
import {UpgradeModule, downgradeComponent} from '@angular/upgrade/static';
13+
import {UpgradeComponent, UpgradeModule, downgradeComponent} from '@angular/upgrade/static';
1414
import * as angular from '@angular/upgrade/static/src/common/angular1';
1515

1616
import {$apply, bootstrap, html, multiTrim, withEachNg1Version} from '../test_helpers';
@@ -461,6 +461,57 @@ withEachNg1Version(() => {
461461
});
462462
}));
463463

464+
it('should properly run cleanup with multiple levels of nesting', async(() => {
465+
let destroyed = false;
466+
467+
@Component({
468+
selector: 'ng2-outer',
469+
template: '<div *ngIf="!destroyIt"><ng1></ng1></div>',
470+
})
471+
class Ng2OuterComponent {
472+
@Input() destroyIt = false;
473+
}
474+
475+
@Component({selector: 'ng2-inner', template: 'test'})
476+
class Ng2InnerComponent implements OnDestroy {
477+
ngOnDestroy() { destroyed = true; }
478+
}
479+
480+
@Directive({selector: 'ng1'})
481+
class Ng1ComponentFacade extends UpgradeComponent {
482+
constructor(elementRef: ElementRef, injector: Injector) {
483+
super('ng1', elementRef, injector);
484+
}
485+
}
486+
487+
@NgModule({
488+
imports: [BrowserModule, UpgradeModule],
489+
declarations: [Ng1ComponentFacade, Ng2InnerComponent, Ng2OuterComponent],
490+
entryComponents: [Ng2InnerComponent, Ng2OuterComponent],
491+
})
492+
class Ng2Module {
493+
ngDoBootstrap() {}
494+
}
495+
496+
const ng1Module =
497+
angular.module('ng1', [])
498+
.directive('ng1', () => ({template: '<ng2-inner></ng2-inner>'}))
499+
.directive('ng2Inner', downgradeComponent({component: Ng2InnerComponent}))
500+
.directive('ng2Outer', downgradeComponent({component: Ng2OuterComponent}));
501+
502+
const element = html('<ng2-outer [destroy-it]="destroyIt"></ng2-outer>');
503+
504+
bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => {
505+
expect(element.textContent).toBe('test');
506+
expect(destroyed).toBe(false);
507+
508+
$apply(upgrade, 'destroyIt = true');
509+
510+
expect(element.textContent).toBe('');
511+
expect(destroyed).toBe(true);
512+
});
513+
}));
514+
464515
it('should work when compiled outside the dom (by fallback to the root ng2.injector)',
465516
async(() => {
466517

0 commit comments

Comments
 (0)