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.

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

fix(ivy): destroy injector when module is destroyed #27793

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions packages/core/src/di/r3_injector.ts
Expand Up @@ -102,7 +102,8 @@ export class R3Injector {
/**
* Flag indicating that this injector was previously destroyed.
*/
private destroyed = false;
get destroyed(): boolean { return this._destroyed; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why turning this into getter?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to prevent people from being able to set it from the outside. It could also be turned into a isDestroyed method which'll generate slightly less ES5 code.

private _destroyed = false;

constructor(
def: InjectorType<any>, additionalProviders: StaticProvider[]|null,
Expand Down Expand Up @@ -138,7 +139,7 @@ export class R3Injector {
this.assertNotDestroyed();

// Set destroyed = true first, in case lifecycle hooks re-enter destroy().
this.destroyed = true;
this._destroyed = true;
try {
// Call all the lifecycle hooks.
this.onDestroy.forEach(service => service.ngOnDestroy());
Expand Down Expand Up @@ -189,7 +190,7 @@ export class R3Injector {
}

private assertNotDestroyed(): void {
if (this.destroyed) {
if (this._destroyed) {
throw new Error('Injector has already been destroyed.');
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/render3/component_ref.ts
Expand Up @@ -276,7 +276,7 @@ export class ComponentRef<T> extends viewEngine_ComponentRef<T> {
ngDevMode && assertDefined(this.destroyCbs, 'NgModule already destroyed');
this.destroyCbs !.forEach(fn => fn());
this.destroyCbs = null;
this.hostView.destroy();
!this.hostView.destroyed && this.hostView.destroy();
}
onDestroy(callback: () => void): void {
ngDevMode && assertDefined(this.destroyCbs, 'NgModule already destroyed');
Expand Down
8 changes: 5 additions & 3 deletions packages/core/src/render3/ng_module_ref.ts
Expand Up @@ -9,7 +9,7 @@
import {INJECTOR, Injector} from '../di/injector';
import {InjectFlags} from '../di/interface/injector';
import {StaticProvider} from '../di/interface/provider';
import {createInjector} from '../di/r3_injector';
import {R3Injector, createInjector} from '../di/r3_injector';
import {Type} from '../interface/type';
import {ComponentFactoryResolver as viewEngine_ComponentFactoryResolver} from '../linker/component_factory_resolver';
import {InternalNgModuleRef, NgModuleFactory as viewEngine_NgModuleFactory, NgModuleRef as viewEngine_NgModuleRef} from '../linker/ng_module_factory';
Expand All @@ -32,7 +32,7 @@ export class NgModuleRef<T> extends viewEngine_NgModuleRef<T> implements Interna
// tslint:disable-next-line:require-internal-with-underscore
_bootstrapComponents: Type<any>[] = [];
// tslint:disable-next-line:require-internal-with-underscore
_r3Injector: Injector;
_r3Injector: R3Injector;
injector: Injector = this;
instance: T;
destroyCbs: (() => void)[]|null = [];
Expand All @@ -52,7 +52,7 @@ export class NgModuleRef<T> extends viewEngine_NgModuleRef<T> implements Interna
},
COMPONENT_FACTORY_RESOLVER
];
this._r3Injector = createInjector(ngModuleType, _parent, additionalProviders);
this._r3Injector = createInjector(ngModuleType, _parent, additionalProviders) as R3Injector;
this.instance = this.get(ngModuleType);
}

Expand All @@ -70,6 +70,8 @@ export class NgModuleRef<T> extends viewEngine_NgModuleRef<T> implements Interna

destroy(): void {
ngDevMode && assertDefined(this.destroyCbs, 'NgModule already destroyed');
const injector = this._r3Injector;
!injector.destroyed && injector.destroy();
this.destroyCbs !.forEach(fn => fn());
this.destroyCbs = null;
}
Expand Down
78 changes: 38 additions & 40 deletions packages/core/test/linker/ng_module_integration_spec.ts
Expand Up @@ -1045,55 +1045,53 @@ function declareTests(config?: {useJit: boolean}) {
expect(created).toBe(false);
});

fixmeIvy('FW-739: TestBed: destroy on NgModuleRef is not being called')
.it('should support ngOnDestroy on any provider', () => {
let destroyed = false;
it('should support ngOnDestroy on any provider', () => {
let destroyed = false;

class SomeInjectable {
ngOnDestroy() { destroyed = true; }
}
class SomeInjectable {
ngOnDestroy() { destroyed = true; }
}

@NgModule({providers: [SomeInjectable]})
class SomeModule {
// Inject SomeInjectable to make it eager...
constructor(i: SomeInjectable) {}
}
@NgModule({providers: [SomeInjectable]})
class SomeModule {
// Inject SomeInjectable to make it eager...
constructor(i: SomeInjectable) {}
}

const moduleRef = createModule(SomeModule);
expect(destroyed).toBe(false);
moduleRef.destroy();
expect(destroyed).toBe(true);
});
const moduleRef = createModule(SomeModule);
expect(destroyed).toBe(false);
moduleRef.destroy();
expect(destroyed).toBe(true);
});

fixmeIvy('FW-739: TestBed: destroy on NgModuleRef is not being called')
.it('should support ngOnDestroy for lazy providers', () => {
let created = false;
let destroyed = false;
it('should support ngOnDestroy for lazy providers', () => {
let created = false;
let destroyed = false;

class SomeInjectable {
constructor() { created = true; }
ngOnDestroy() { destroyed = true; }
}
class SomeInjectable {
constructor() { created = true; }
ngOnDestroy() { destroyed = true; }
}

@NgModule({providers: [SomeInjectable]})
class SomeModule {
}
@NgModule({providers: [SomeInjectable]})
class SomeModule {
}

let moduleRef = createModule(SomeModule);
expect(created).toBe(false);
expect(destroyed).toBe(false);
let moduleRef = createModule(SomeModule);
expect(created).toBe(false);
expect(destroyed).toBe(false);

// no error if the provider was not yet created
moduleRef.destroy();
expect(created).toBe(false);
expect(destroyed).toBe(false);
// no error if the provider was not yet created
moduleRef.destroy();
expect(created).toBe(false);
expect(destroyed).toBe(false);

moduleRef = createModule(SomeModule);
moduleRef.injector.get(SomeInjectable);
expect(created).toBe(true);
moduleRef.destroy();
expect(destroyed).toBe(true);
});
moduleRef = createModule(SomeModule);
moduleRef.injector.get(SomeInjectable);
expect(created).toBe(true);
moduleRef.destroy();
expect(destroyed).toBe(true);
});
});

describe('imported and exported modules', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/upgrade/src/common/downgrade_component_adapter.ts
Expand Up @@ -213,15 +213,15 @@ export class DowngradeComponentAdapter {
}

registerCleanup() {
const testabilityRegistry = this.componentRef.injector.get(TestabilityRegistry);
const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy());
let destroyed = false;

this.element.on !('$destroy', () => this.componentScope.$destroy());
this.componentScope.$on('$destroy', () => {
if (!destroyed) {
destroyed = true;
this.componentRef.injector.get(TestabilityRegistry)
.unregisterApplication(this.componentRef.location.nativeElement);
testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement);
destroyComponentRef();
}
});
Expand Down