From b0befd7376fca057ec08e59086a55b3b3aa4d9ef Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Wed, 4 Oct 2017 09:13:22 -0700 Subject: [PATCH] fix(compiler): `TestBed.overrideProvider` should keep imported `NgModule`s eager (#19624) Before, as soon as a user called `TestBed.overrideProvider` for a provider of a `NgModule` that was imported via `TestBed.configureTestingModule`, that `NgModule` became lazy. This commit changes this behavior to keep the `NgModule` eager, with or without a call to `TestBed.overrideProvider`. PR Close #19624 --- packages/core/src/view/services.ts | 34 +++++++++++----- packages/core/src/view/types.ts | 1 + packages/core/testing/src/test_bed.ts | 2 +- .../test/testing_public_spec.ts | 40 +++++++++++++++++++ 4 files changed, 65 insertions(+), 12 deletions(-) diff --git a/packages/core/src/view/services.ts b/packages/core/src/view/services.ts index 3006195802..6599e808f5 100644 --- a/packages/core/src/view/services.ts +++ b/packages/core/src/view/services.ts @@ -209,9 +209,6 @@ function applyProviderOverridesToView(def: ViewDefinition): ViewDefinition { return; } if (nodeDef.flags & NodeFlags.CatProviderNoDirective) { - // Make all providers lazy, so that we don't get into trouble - // with ordering problems of providers on the same element - nodeDef.flags |= NodeFlags.LazyProvider; const provider = nodeDef.provider !; const override = providerOverrides.get(provider.token); if (override) { @@ -228,7 +225,8 @@ function applyProviderOverridesToView(def: ViewDefinition): ViewDefinition { // We only create new datastructures if we need to, to keep perf impact // reasonable. function applyProviderOverridesToNgModule(def: NgModuleDefinition): NgModuleDefinition { - if (providerOverrides.size === 0 || !hasOverrrides(def)) { + const {hasOverrides, hasDeprecatedOverrides} = calcHasOverrides(def); + if (!hasOverrides) { return def; } // clone the whole view definition, @@ -237,18 +235,32 @@ function applyProviderOverridesToNgModule(def: NgModuleDefinition): NgModuleDefi applyProviderOverrides(def); return def; - function hasOverrrides(def: NgModuleDefinition): boolean { - return def.providers.some( - node => - !!(node.flags & NodeFlags.CatProviderNoDirective) && providerOverrides.has(node.token)); + function calcHasOverrides(def: NgModuleDefinition): + {hasOverrides: boolean, hasDeprecatedOverrides: boolean} { + let hasOverrides = false; + let hasDeprecatedOverrides = false; + if (providerOverrides.size === 0) { + return {hasOverrides, hasDeprecatedOverrides}; + } + def.providers.forEach(node => { + const override = providerOverrides.get(node.token); + if ((node.flags & NodeFlags.CatProviderNoDirective) && override) { + hasOverrides = true; + hasDeprecatedOverrides = hasDeprecatedOverrides || override.deprecatedBehavior; + } + }); + return {hasOverrides, hasDeprecatedOverrides}; } function applyProviderOverrides(def: NgModuleDefinition) { for (let i = 0; i < def.providers.length; i++) { const provider = def.providers[i]; - // Make all providers lazy, so that we don't get into trouble - // with ordering problems of providers on the same element - provider.flags |= NodeFlags.LazyProvider; + if (hasDeprecatedOverrides) { + // We had a bug where me made + // all providers lazy. Keep this logic behind a flag + // for migrating existing users. + provider.flags |= NodeFlags.LazyProvider; + } const override = providerOverrides.get(provider.token); if (override) { provider.flags = (provider.flags & ~NodeFlags.CatProviderNoDirective) | override.flags; diff --git a/packages/core/src/view/types.ts b/packages/core/src/view/types.ts index 011578a687..0a2a2ef030 100644 --- a/packages/core/src/view/types.ts +++ b/packages/core/src/view/types.ts @@ -506,6 +506,7 @@ export interface ProviderOverride { flags: NodeFlags; value: any; deps: ([DepFlags, any]|any)[]; + deprecatedBehavior: boolean; } export interface Services { diff --git a/packages/core/testing/src/test_bed.ts b/packages/core/testing/src/test_bed.ts index 54fc3ebe60..5eaa822063 100644 --- a/packages/core/testing/src/test_bed.ts +++ b/packages/core/testing/src/test_bed.ts @@ -467,7 +467,7 @@ export class TestBed implements Injector { } return [depFlags, depToken]; }); - overrideProvider({token, flags, deps, value}); + overrideProvider({token, flags, deps, value, deprecatedBehavior: deprecated}); } createComponent(component: Type): ComponentFixture { diff --git a/packages/platform-browser/test/testing_public_spec.ts b/packages/platform-browser/test/testing_public_spec.ts index cd97306f7e..609f659bbc 100644 --- a/packages/platform-browser/test/testing_public_spec.ts +++ b/packages/platform-browser/test/testing_public_spec.ts @@ -468,6 +468,46 @@ export function main() { expect(modFactory.create(getTestBed()).injector.get('a')).toBe('mockA: parentDepValue'); }); + it('should keep imported NgModules eager', () => { + let someModule: SomeModule|undefined; + + @NgModule() + class SomeModule { + constructor() { someModule = this; } + } + + TestBed.configureTestingModule({ + providers: [ + {provide: 'a', useValue: 'aValue'}, + ], + imports: [SomeModule] + }); + TestBed.overrideProvider('a', {useValue: 'mockValue'}); + + expect(TestBed.get('a')).toBe('mockValue'); + expect(someModule).toBeAnInstanceOf(SomeModule); + }); + + it('should keep imported NgModules lazy with deprecatedOverrideProvider', () => { + let someModule: SomeModule|undefined; + + @NgModule() + class SomeModule { + constructor() { someModule = this; } + } + + TestBed.configureTestingModule({ + providers: [ + {provide: 'a', useValue: 'aValue'}, + ], + imports: [SomeModule] + }); + TestBed.deprecatedOverrideProvider('a', {useValue: 'mockValue'}); + + expect(TestBed.get('a')).toBe('mockValue'); + expect(someModule).toBeUndefined(); + }); + describe('injecting eager providers into an eager overwritten provider', () => { @NgModule({ providers: [