fix: don't instantiate providers with ngOnDestroy eagerly. (#15070)

BREAKING CHANGE:

Perviously, any provider that had an ngOnDestroy lifecycle hook would be created eagerly.

Now, only classes that are annotated with @Component, @Directive, @Pipe, @NgModule are eager. Providers only become eager if they are either directly or transitively injected into one of the above.

This also makes all `useValue` providers eager, which
should have no observable impact other than code size.

EXPECTED IMPACT:
Making providers eager was an incorrect behavior and never documented.
Also, providers that are used by a directive / pipe / ngModule stay eager.
So the impact should be rather small.

Fixes #14552
This commit is contained in:
Tobias Bosch 2017-03-14 14:32:26 -07:00 committed by Chuck Jazdzewski
parent 0aad270267
commit 2c5a671341
10 changed files with 75 additions and 35 deletions

View File

@ -84,6 +84,7 @@ class _InjectorBuilder implements ClassBuilder {
getters: o.ClassGetter[] = [];
methods: o.ClassMethod[] = [];
ctorStmts: o.Statement[] = [];
private _lazyProps = new Map<string, o.Expression>();
private _tokens: CompileTokenMetadata[] = [];
private _instances = new Map<any, o.Expression>();
private _createStmts: o.Statement[] = [];
@ -103,7 +104,11 @@ class _InjectorBuilder implements ClassBuilder {
propName, resolvedProvider, providerValueExpressions, resolvedProvider.multiProvider,
resolvedProvider.eager);
if (resolvedProvider.lifecycleHooks.indexOf(ɵLifecycleHooks.OnDestroy) !== -1) {
this._destroyStmts.push(instance.callMethod('ngOnDestroy', []).toStmt());
let callNgOnDestroy: o.Expression = instance.callMethod('ngOnDestroy', []);
if (!resolvedProvider.eager) {
callNgOnDestroy = this._lazyProps.get(instance.name).and(callNgOnDestroy);
}
this._destroyStmts.push(callNgOnDestroy.toStmt());
}
this._tokens.push(resolvedProvider.token);
this._instances.set(tokenReference(resolvedProvider.token), instance);
@ -173,7 +178,7 @@ class _InjectorBuilder implements ClassBuilder {
private _createProviderProperty(
propName: string, provider: ProviderAst, providerValueExpressions: o.Expression[],
isMulti: boolean, isEager: boolean): o.Expression {
isMulti: boolean, isEager: boolean): o.ReadPropExpr {
let resolvedProviderValueExpr: o.Expression;
let type: o.Type;
if (isMulti) {
@ -190,16 +195,17 @@ class _InjectorBuilder implements ClassBuilder {
this.fields.push(new o.ClassField(propName, type));
this._createStmts.push(o.THIS_EXPR.prop(propName).set(resolvedProviderValueExpr).toStmt());
} else {
const internalField = `_${propName}`;
this.fields.push(new o.ClassField(internalField, type));
const internalFieldProp = o.THIS_EXPR.prop(`_${propName}`);
this.fields.push(new o.ClassField(internalFieldProp.name, type));
// Note: Equals is important for JS so that it also checks the undefined case!
const getterStmts = [
new o.IfStmt(
o.THIS_EXPR.prop(internalField).isBlank(),
[o.THIS_EXPR.prop(internalField).set(resolvedProviderValueExpr).toStmt()]),
new o.ReturnStatement(o.THIS_EXPR.prop(internalField))
internalFieldProp.isBlank(),
[internalFieldProp.set(resolvedProviderValueExpr).toStmt()]),
new o.ReturnStatement(internalFieldProp)
];
this.getters.push(new o.ClassGetter(propName, getterStmts, type));
this._lazyProps.set(propName, internalFieldProp);
}
return o.THIS_EXPR.prop(propName);
}

View File

@ -462,9 +462,10 @@ function _resolveProviders(
(<CompileTypeMetadata>provider.token.identifier).lifecycleHooks ?
(<CompileTypeMetadata>provider.token.identifier).lifecycleHooks :
[];
const isUseValue = !(provider.useClass || provider.useExisting || provider.useFactory);
resolvedProvider = new ProviderAst(
provider.token, provider.multi, eager || lifecycleHooks.length > 0, [provider],
providerType, lifecycleHooks, sourceSpan);
provider.token, provider.multi, eager || isUseValue, [provider], providerType,
lifecycleHooks, sourceSpan);
targetProvidersByToken.set(tokenReference(provider.token), resolvedProvider);
} else {
if (!provider.multi) {

View File

@ -56,4 +56,6 @@ export function _initViewEngine() {
]
})
export class ApplicationModule {
// Inject ApplicationRef to make it eager...
constructor(appRef: ApplicationRef) {}
}

View File

@ -468,6 +468,9 @@ function callElementProvidersLifecycles(view: ViewData, elDef: NodeDef, lifecycl
function callProviderLifecycles(view: ViewData, index: number, lifecycles: NodeFlags) {
const provider = asProviderData(view, index).instance;
if (provider === NOT_CREATED) {
return;
}
Services.setCurrentNode(view, index);
if (lifecycles & NodeFlags.AfterContentInit) {
provider.ngAfterContentInit();

View File

@ -907,6 +907,8 @@ function declareTests({useJit}: {useJit: boolean}) {
@NgModule({providers: [SomeInjectable]})
class SomeModule {
// Inject SomeInjectable to make it eager...
constructor(i: SomeInjectable) {}
}
const moduleRef = createModule(SomeModule);
@ -915,17 +917,33 @@ function declareTests({useJit}: {useJit: boolean}) {
expect(destroyed).toBe(true);
});
it('should instantiate providers with lifecycle eagerly', () => {
it('should support ngOnDestroy for lazy providers', () => {
let created = false;
let destroyed = false;
class SomeInjectable {
constructor() { created = true; }
ngOnDestroy() {}
ngOnDestroy() { destroyed = true; }
}
createInjector([SomeInjectable]);
@NgModule({providers: [SomeInjectable]})
class SomeModule {
}
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);
moduleRef = createModule(SomeModule);
moduleRef.injector.get(SomeInjectable);
expect(created).toBe(true);
moduleRef.destroy();
expect(destroyed).toBe(true);
});
});

View File

@ -342,34 +342,36 @@ export function main() {
expect(created).toBe(true);
});
it('should instantiate providers lazily', () => {
TestBed.configureTestingModule({declarations: [SimpleDirective]});
it('should support ngOnDestroy for lazy providers', () => {
let created = false;
TestBed.overrideDirective(
SimpleDirective,
{set: {providers: [{provide: 'service', useFactory: () => created = true}]}});
let destroyed = false;
const el = createComponent('<div simpleDirective></div>');
expect(created).toBe(false);
el.children[0].injector.get('service');
expect(created).toBe(true);
});
it('should instantiate providers with a lifecycle hook eagerly', () => {
let created = false;
class SomeInjectable {
constructor() { created = true; }
ngOnDestroy() {}
ngOnDestroy() { destroyed = true; }
}
TestBed.configureTestingModule({declarations: [SimpleDirective]});
TestBed.overrideDirective(SimpleDirective, {set: {providers: [SomeInjectable]}});
const el = createComponent('<div simpleDirective></div>');
@Component({providers: [SomeInjectable], template: ''})
class SomeComp {
}
TestBed.configureTestingModule({declarations: [SomeComp]});
let compRef = TestBed.createComponent(SomeComp).componentRef;
expect(created).toBe(false);
expect(destroyed).toBe(false);
// no error if the provider was not yet created
compRef.destroy();
expect(created).toBe(false);
expect(destroyed).toBe(false);
compRef = TestBed.createComponent(SomeComp).componentRef;
compRef.injector.get(SomeInjectable);
expect(created).toBe(true);
compRef.destroy();
expect(destroyed).toBe(true);
});
it('should instantiate view providers lazily', () => {

View File

@ -213,7 +213,13 @@ export class TestBed implements Injector {
this._imports = [];
this._schemas = [];
this._instantiated = false;
this._activeFixtures.forEach((fixture) => fixture.destroy());
this._activeFixtures.forEach((fixture) => {
try {
fixture.destroy();
} catch (e) {
console.error('Error during cleanup of component', fixture.componentInstance);
}
});
this._activeFixtures = [];
}

View File

@ -123,7 +123,8 @@ export function routerNgProbeToken() {
*/
@NgModule({declarations: ROUTER_DIRECTIVES, exports: ROUTER_DIRECTIVES})
export class RouterModule {
constructor(@Optional() @Inject(ROUTER_FORROOT_GUARD) guard: any) {}
// Note: We are injecting the Router so it gets created eagerly...
constructor(@Optional() @Inject(ROUTER_FORROOT_GUARD) guard: any, @Optional() router: Router) {}
/**
* Creates a module with all the router providers and directives. It also optionally sets up an

View File

@ -121,6 +121,7 @@ export declare class ApplicationInitStatus {
/** @experimental */
export declare class ApplicationModule {
constructor(appRef: ApplicationRef);
}
/** @stable */

View File

@ -308,7 +308,7 @@ export declare class RouterLinkWithHref implements OnChanges, OnDestroy {
/** @stable */
export declare class RouterModule {
constructor(guard: any);
constructor(guard: any, router: Router);
static forChild(routes: Routes): ModuleWithProviders;
static forRoot(routes: Routes, config?: ExtraOptions): ModuleWithProviders;
}