From 6ade68cff1bd0146813fee48d9a6042d85a0859e Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Wed, 4 Oct 2017 09:13:22 -0700 Subject: [PATCH] fix(compiler): correctly instantiate eager providers that are used via `Injector.get` (#19558) Closes #15501 PR Close #19558 --- packages/core/src/view/ng_module.ts | 27 +++++--- packages/core/src/view/provider.ts | 19 +++--- packages/core/src/view/view.ts | 16 +++-- .../test/linker/ng_module_integration_spec.ts | 57 +++++++++++++++++ .../linker/view_injector_integration_spec.ts | 62 +++++++++++++++++++ .../worker/renderer_v2_integration_spec.ts | 6 +- 6 files changed, 163 insertions(+), 24 deletions(-) diff --git a/packages/core/src/view/ng_module.ts b/packages/core/src/view/ng_module.ts index e8d9f485d2..5c57bf278e 100644 --- a/packages/core/src/view/ng_module.ts +++ b/packages/core/src/view/ng_module.ts @@ -13,7 +13,7 @@ import {NgModuleRef} from '../linker/ng_module_factory'; import {DepDef, DepFlags, NgModuleData, NgModuleDefinition, NgModuleProviderDef, NodeFlags} from './types'; import {splitDepsDsl, tokenKey} from './util'; -const NOT_CREATED = new Object(); +const UNDEFINED_VALUE = new Object(); const InjectorRefTokenKey = tokenKey(Injector); const NgModuleRefTokenKey = tokenKey(NgModuleRef); @@ -53,8 +53,9 @@ export function initNgModule(data: NgModuleData) { const providers = data._providers = new Array(def.providers.length); for (let i = 0; i < def.providers.length; i++) { const provDef = def.providers[i]; - providers[i] = provDef.flags & NodeFlags.LazyProvider ? NOT_CREATED : - _createProviderInstance(data, provDef); + if (!(provDef.flags & NodeFlags.LazyProvider)) { + providers[i] = _createProviderInstance(data, provDef); + } } } @@ -78,27 +79,33 @@ export function resolveNgModuleDep( const providerDef = data._def.providersByKey[tokenKey]; if (providerDef) { let providerInstance = data._providers[providerDef.index]; - if (providerInstance === NOT_CREATED) { + if (providerInstance === undefined) { providerInstance = data._providers[providerDef.index] = _createProviderInstance(data, providerDef); } - return providerInstance; + return providerInstance === UNDEFINED_VALUE ? undefined : providerInstance; } return data._parent.get(depDef.token, notFoundValue); } function _createProviderInstance(ngModule: NgModuleData, providerDef: NgModuleProviderDef): any { + let injectable: any; switch (providerDef.flags & NodeFlags.Types) { case NodeFlags.TypeClassProvider: - return _createClass(ngModule, providerDef.value, providerDef.deps); + injectable = _createClass(ngModule, providerDef.value, providerDef.deps); + break; case NodeFlags.TypeFactoryProvider: - return _callFactory(ngModule, providerDef.value, providerDef.deps); + injectable = _callFactory(ngModule, providerDef.value, providerDef.deps); + break; case NodeFlags.TypeUseExistingProvider: - return resolveNgModuleDep(ngModule, providerDef.deps[0]); + injectable = resolveNgModuleDep(ngModule, providerDef.deps[0]); + break; case NodeFlags.TypeValueProvider: - return providerDef.value; + injectable = providerDef.value; + break; } + return injectable === undefined ? UNDEFINED_VALUE : injectable; } function _createClass(ngModule: NgModuleData, ctor: any, deps: DepDef[]): any { @@ -151,7 +158,7 @@ export function callNgModuleLifecycle(ngModule: NgModuleData, lifecycles: NodeFl const provDef = def.providers[i]; if (provDef.flags & NodeFlags.OnDestroy) { const instance = ngModule._providers[i]; - if (instance && instance !== NOT_CREATED) { + if (instance && instance !== UNDEFINED_VALUE) { instance.ngOnDestroy(); } } diff --git a/packages/core/src/view/provider.ts b/packages/core/src/view/provider.ts index a4a810d7ed..f8ae12049c 100644 --- a/packages/core/src/view/provider.ts +++ b/packages/core/src/view/provider.ts @@ -25,8 +25,6 @@ const TemplateRefTokenKey = tokenKey(TemplateRef); const ChangeDetectorRefTokenKey = tokenKey(ChangeDetectorRef); const InjectorRefTokenKey = tokenKey(Injector); -const NOT_CREATED = new Object(); - export function directiveDef( checkIndex: number, flags: NodeFlags, matchedQueries: null | [string | number, QueryValueType][], childCount: number, ctor: any, @@ -111,7 +109,7 @@ export function _def( } export function createProviderInstance(view: ViewData, def: NodeDef): any { - return def.flags & NodeFlags.LazyProvider ? NOT_CREATED : _createProviderInstance(view, def); + return _createProviderInstance(view, def); } export function createPipeInstance(view: ViewData, def: NodeDef): any { @@ -378,9 +376,10 @@ export function resolveDep( (allowPrivateServices ? elDef.element !.allProviders : elDef.element !.publicProviders) ![tokenKey]; if (providerDef) { - const providerData = asProviderData(view, providerDef.nodeIndex); - if (providerData.instance === NOT_CREATED) { - providerData.instance = _createProviderInstance(view, providerDef); + let providerData = asProviderData(view, providerDef.nodeIndex); + if (!providerData) { + providerData = {instance: _createProviderInstance(view, providerDef)}; + view.nodes[providerDef.nodeIndex] = providerData as any; } return providerData.instance; } @@ -487,8 +486,12 @@ 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) { + const providerData = asProviderData(view, index); + if (!providerData) { + return; + } + const provider = providerData.instance; + if (!provider) { return; } Services.setCurrentNode(view, index); diff --git a/packages/core/src/view/view.ts b/packages/core/src/view/view.ts index ad86d558d8..d37a3106b1 100644 --- a/packages/core/src/view/view.ts +++ b/packages/core/src/view/view.ts @@ -284,8 +284,11 @@ function createViewNodes(view: ViewData) { case NodeFlags.TypeFactoryProvider: case NodeFlags.TypeUseExistingProvider: case NodeFlags.TypeValueProvider: { - const instance = createProviderInstance(view, nodeDef); - nodeData = {instance}; + nodeData = nodes[i]; + if (!nodeData && !(nodeDef.flags & NodeFlags.LazyProvider)) { + const instance = createProviderInstance(view, nodeDef); + nodeData = {instance}; + } break; } case NodeFlags.TypePipe: { @@ -294,11 +297,14 @@ function createViewNodes(view: ViewData) { break; } case NodeFlags.TypeDirective: { - const instance = createDirectiveInstance(view, nodeDef); - nodeData = {instance}; + nodeData = nodes[i]; + if (!nodeData) { + const instance = createDirectiveInstance(view, nodeDef); + nodeData = {instance}; + } if (nodeDef.flags & NodeFlags.Component) { const compView = asElementData(view, nodeDef.parent !.nodeIndex).componentView; - initView(compView, instance, instance); + initView(compView, nodeData.instance, nodeData.instance); } break; } diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts index 56660ed27e..619faf3ab0 100644 --- a/packages/core/test/linker/ng_module_integration_spec.ts +++ b/packages/core/test/linker/ng_module_integration_spec.ts @@ -787,6 +787,22 @@ function declareTests({useJit}: {useJit: boolean}) { expect(child.get(Injector)).toBe(child); }); + it('should provide undefined', () => { + let factoryCounter = 0; + + const injector = createInjector([{ + provide: 'token', + useFactory: () => { + factoryCounter++; + return undefined; + } + }]); + + expect(injector.get('token')).toBeUndefined(); + expect(injector.get('token')).toBeUndefined(); + expect(factoryCounter).toBe(1); + }); + describe('injecting lazy providers into an eager provider via Injector.get', () => { it('should inject providers that were declared before it', () => { @@ -828,6 +844,47 @@ function declareTests({useJit}: {useJit: boolean}) { }); }); + describe('injecting eager providers into an eager provider via Injector.get', () => { + + it('should inject providers that were declared before it', () => { + @NgModule({ + providers: [ + {provide: 'eager1', useFactory: () => 'v1'}, + { + provide: 'eager2', + useFactory: (i: Injector) => `v2: ${i.get('eager1')}`, + deps: [Injector] + }, + ] + }) + class MyModule { + // NgModule is eager, which makes all of its deps eager + constructor(@Inject('eager1') eager1: any, @Inject('eager2') eager2: any) {} + } + + expect(createModule(MyModule).injector.get('eager2')).toBe('v2: v1'); + }); + + it('should inject providers that were declared after it', () => { + @NgModule({ + providers: [ + { + provide: 'eager1', + useFactory: (i: Injector) => `v1: ${i.get('eager2')}`, + deps: [Injector] + }, + {provide: 'eager2', useFactory: () => 'v2'}, + ] + }) + class MyModule { + // NgModule is eager, which makes all of its deps eager + constructor(@Inject('eager1') eager1: any, @Inject('eager2') eager2: any) {} + } + + expect(createModule(MyModule).injector.get('eager1')).toBe('v1: v2'); + }); + }); + it('should throw when no provider defined', () => { const injector = createInjector([]); expect(() => injector.get('NonExisting')) diff --git a/packages/core/test/linker/view_injector_integration_spec.ts b/packages/core/test/linker/view_injector_integration_spec.ts index 96e3f6605f..9df00ad40c 100644 --- a/packages/core/test/linker/view_injector_integration_spec.ts +++ b/packages/core/test/linker/view_injector_integration_spec.ts @@ -342,6 +342,22 @@ export function main() { expect(created).toBe(true); }); + it('should provide undefined', () => { + let factoryCounter = 0; + + const el = createComponent('', [{ + provide: 'token', + useFactory: () => { + factoryCounter++; + return undefined; + } + }]); + + expect(el.injector.get('token')).toBeUndefined(); + expect(el.injector.get('token')).toBeUndefined(); + expect(factoryCounter).toBe(1); + }); + describe('injecting lazy providers into an eager provider via Injector.get', () => { it('should inject providers that were declared before it', () => { @@ -389,6 +405,52 @@ export function main() { }); }); + describe('injecting eager providers into an eager provider via Injector.get', () => { + it('should inject providers that were declared before it', () => { + @Component({ + template: '', + providers: [ + {provide: 'eager1', useFactory: () => 'v1'}, + { + provide: 'eager2', + useFactory: (i: Injector) => `v2: ${i.get('eager1')}`, + deps: [Injector] + }, + ] + }) + class MyComp { + // Component is eager, which makes all of its deps eager + constructor(@Inject('eager1') eager1: any, @Inject('eager2') eager2: any) {} + } + + const ctx = + TestBed.configureTestingModule({declarations: [MyComp]}).createComponent(MyComp); + expect(ctx.debugElement.injector.get('eager2')).toBe('v2: v1'); + }); + + it('should inject providers that were declared after it', () => { + @Component({ + template: '', + providers: [ + { + provide: 'eager1', + useFactory: (i: Injector) => `v1: ${i.get('eager2')}`, + deps: [Injector] + }, + {provide: 'eager2', useFactory: () => 'v2'}, + ] + }) + class MyComp { + // Component is eager, which makes all of its deps eager + constructor(@Inject('eager1') eager1: any, @Inject('eager2') eager2: any) {} + } + + const ctx = + TestBed.configureTestingModule({declarations: [MyComp]}).createComponent(MyComp); + expect(ctx.debugElement.injector.get('eager1')).toBe('v1: v2'); + }); + }); + it('should allow injecting lazy providers via Injector.get from an eager provider that is declared earlier', () => { @Component({providers: [{provide: 'a', useFactory: () => 'aValue'}], template: ''}) diff --git a/packages/platform-webworker/test/web_workers/worker/renderer_v2_integration_spec.ts b/packages/platform-webworker/test/web_workers/worker/renderer_v2_integration_spec.ts index dc3e3045fb..16a26462ff 100644 --- a/packages/platform-webworker/test/web_workers/worker/renderer_v2_integration_spec.ts +++ b/packages/platform-webworker/test/web_workers/worker/renderer_v2_integration_spec.ts @@ -12,7 +12,7 @@ import {platformBrowserDynamicTesting} from '@angular/platform-browser-dynamic/t import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {DomRendererFactory2} from '@angular/platform-browser/src/dom/dom_renderer'; import {BrowserTestingModule} from '@angular/platform-browser/testing'; -import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util'; +import {browserDetection, dispatchEvent} from '@angular/platform-browser/testing/src/browser_util'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {ClientMessageBrokerFactory} from '../../../src/web_workers/shared/client_message_broker'; @@ -29,6 +29,10 @@ export function main() { describe('Web Worker Renderer v2', () => { // Don't run on server... if (!getDOM().supportsDOMEvents()) return; + // TODO(tbosch): investigate why this is failing on iOS7 for unrelated reasons + // Note: it's hard to debug this as SauceLabs starts with iOS8. Maybe drop + // iOS7 alltogether? + if (browserDetection.isIOS7) return; let uiRenderStore: RenderStore; let wwRenderStore: RenderStore;