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
angular · Mar 14, 2017 · 2c5a671 · 2c5a671
1 parent 0aad270
commit 2c5a671
Show file tree

Hide file tree

Showing 10 changed files with 74 additions and 34 deletions.
diff --git a/packages/compiler/src/ng_module_compiler.ts b/packages/compiler/src/ng_module_compiler.ts
@@ -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);
   }

diff --git a/packages/compiler/src/provider_analyzer.ts b/packages/compiler/src/provider_analyzer.ts
@@ -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) {

diff --git a/packages/core/src/application_module.ts b/packages/core/src/application_module.ts
@@ -56,4 +56,6 @@ export function _initViewEngine() {
   ]
 })
 export class ApplicationModule {
+  // Inject ApplicationRef to make it eager...
+  constructor(appRef: ApplicationRef) {}
 }
diff --git a/packages/core/src/view/provider.ts b/packages/core/src/view/provider.ts
@@ -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();

diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts
@@ -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);
         });
       });
 

diff --git a/packages/core/test/linker/view_injector_integration_spec.ts b/packages/core/test/linker/view_injector_integration_spec.ts
@@ -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>');
+        class SomeInjectable {
+          constructor() { created = true; }
+          ngOnDestroy() { destroyed = true; }
+        }
 
-        expect(created).toBe(false);
+        @Component({providers: [SomeInjectable], template: ''})
+        class SomeComp {
+        }
 
-        el.children[0].injector.get('service');
+        TestBed.configureTestingModule({declarations: [SomeComp]});
 
-        expect(created).toBe(true);
-      });
 
-      it('should instantiate providers with a lifecycle hook eagerly', () => {
-        let created = false;
-        class SomeInjectable {
-          constructor() { created = true; }
-          ngOnDestroy() {}
-        }
-        TestBed.configureTestingModule({declarations: [SimpleDirective]});
-        TestBed.overrideDirective(SimpleDirective, {set: {providers: [SomeInjectable]}});
+        let compRef = TestBed.createComponent(SomeComp).componentRef;
+        expect(created).toBe(false);
+        expect(destroyed).toBe(false);
 
-        const el = createComponent('<div simpleDirective></div>');
+        // 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', () => {

diff --git a/packages/core/testing/src/test_bed.ts b/packages/core/testing/src/test_bed.ts
@@ -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 = [];
   }
 

diff --git a/packages/router/src/router_module.ts b/packages/router/src/router_module.ts
@@ -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

diff --git a/tools/public_api_guard/core/typings/core.d.ts b/tools/public_api_guard/core/typings/core.d.ts
@@ -121,6 +121,7 @@ export declare class ApplicationInitStatus {
 
 /** @experimental */
 export declare class ApplicationModule {
+    constructor(appRef: ApplicationRef);
 }
 
 /** @stable */

diff --git a/tools/public_api_guard/router/typings/router.d.ts b/tools/public_api_guard/router/typings/router.d.ts
@@ -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;
 }