From 0bc35a71e2eaa0c57ef821245755c352305dd3fc Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 22 Feb 2020 11:18:33 +0100 Subject: [PATCH] fix(ivy): injecting incorrect provider when re-providing injectable with useClass (#34574) If an injectable has a `useClass`, Ivy injects the token in `useClass`, rather than the original injectable, if the injectable is re-provided under a different token. The correct behavior is that it should inject the re-provided token, no matter whether it has `useClass`. Fixes #34110. PR Close #34574 --- packages/core/src/di/r3_injector.ts | 8 +- packages/core/test/acceptance/di_spec.ts | 106 ++++++++++++++++++ .../core/test/acceptance/providers_spec.ts | 38 +++---- 3 files changed, 130 insertions(+), 22 deletions(-) diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index 132c3891fb..cd6ccb6071 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -12,6 +12,7 @@ import {OnDestroy} from '../interface/lifecycle_hooks'; import {Type} from '../interface/type'; import {getFactoryDef} from '../render3/definition'; import {throwCyclicDependencyError, throwInvalidProviderError, throwMixedMultiProviderError} from '../render3/errors'; +import {FactoryFn} from '../render3/interfaces/definition'; import {deepForEach, newArray} from '../util/array_utils'; import {stringify} from '../util/stringify'; import {resolveForwardRef} from './forward_ref'; @@ -407,7 +408,7 @@ export class R3Injector { } } -function injectableDefOrInjectorDefFactory(token: Type| InjectionToken): () => any { +function injectableDefOrInjectorDefFactory(token: Type| InjectionToken): FactoryFn { // Most tokens will have an injectable def directly on them, which specifies a factory directly. const injectableDef = getInjectableDef(token); const factory = injectableDef !== null ? injectableDef.factory : getFactoryDef(token); @@ -478,7 +479,8 @@ export function providerToFactory( provider: SingleProvider, ngModuleType?: InjectorType, providers?: any[]): () => any { let factory: (() => any)|undefined = undefined; if (isTypeProvider(provider)) { - return injectableDefOrInjectorDefFactory(resolveForwardRef(provider)); + const unwrappedProvider = resolveForwardRef(provider); + return getFactoryDef(unwrappedProvider) || injectableDefOrInjectorDefFactory(unwrappedProvider); } else { if (isValueProvider(provider)) { factory = () => resolveForwardRef(provider.useValue); @@ -496,7 +498,7 @@ export function providerToFactory( if (hasDeps(provider)) { factory = () => new (classRef)(...injectArgs(provider.deps)); } else { - return injectableDefOrInjectorDefFactory(classRef); + return getFactoryDef(classRef) || injectableDefOrInjectorDefFactory(classRef); } } } diff --git a/packages/core/test/acceptance/di_spec.ts b/packages/core/test/acceptance/di_spec.ts index fa8ed2783c..23d319de03 100644 --- a/packages/core/test/acceptance/di_spec.ts +++ b/packages/core/test/acceptance/di_spec.ts @@ -1066,6 +1066,112 @@ describe('di', () => { }); + describe('service injection with useClass', () => { + @Injectable({providedIn: 'root'}) + class BarServiceDep { + name = 'BarServiceDep'; + } + + @Injectable({providedIn: 'root'}) + class BarService { + constructor(public dep: BarServiceDep) {} + getMessage() { return 'bar'; } + } + + @Injectable({providedIn: 'root'}) + class FooServiceDep { + name = 'FooServiceDep'; + } + + @Injectable({providedIn: 'root', useClass: BarService}) + class FooService { + constructor(public dep: FooServiceDep) {} + getMessage() { return 'foo'; } + } + + it('should use @Injectable useClass config when token is not provided', () => { + let provider: FooService|BarService; + + @Component({template: ''}) + class App { + constructor(service: FooService) { provider = service; } + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(provider !.getMessage()).toBe('bar'); + + // ViewEngine incorrectly uses the original class DI config, instead of the one from useClass. + if (ivyEnabled) { + expect(provider !.dep.name).toBe('BarServiceDep'); + } + }); + + it('should use constructor config directly when token is explicitly provided via useClass', + () => { + let provider: FooService|BarService; + + @Component({template: ''}) + class App { + constructor(service: FooService) { provider = service; } + } + + TestBed.configureTestingModule( + {declarations: [App], providers: [{provide: FooService, useClass: FooService}]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(provider !.getMessage()).toBe('foo'); + }); + + + it('should inject correct provider when re-providing an injectable that has useClass', () => { + let directProvider: FooService|BarService; + let overriddenProvider: FooService|BarService; + + @Component({template: ''}) + class App { + constructor(@Inject('stringToken') overriddenService: FooService, service: FooService) { + overriddenProvider = overriddenService; + directProvider = service; + } + } + + TestBed.configureTestingModule( + {declarations: [App], providers: [{provide: 'stringToken', useClass: FooService}]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(directProvider !.getMessage()).toBe('bar'); + expect(overriddenProvider !.getMessage()).toBe('foo'); + + // ViewEngine incorrectly uses the original class DI config, instead of the one from useClass. + if (ivyEnabled) { + expect(directProvider !.dep.name).toBe('BarServiceDep'); + expect(overriddenProvider !.dep.name).toBe('FooServiceDep'); + } + }); + + it('should use constructor config directly when token is explicitly provided as a type provider', + () => { + let provider: FooService|BarService; + + @Component({template: ''}) + class App { + constructor(service: FooService) { provider = service; } + } + + TestBed.configureTestingModule({declarations: [App], providers: [FooService]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(provider !.getMessage()).toBe('foo'); + expect(provider !.dep.name).toBe('FooServiceDep'); + }); + }); + describe('inject', () => { it('should inject from parent view', () => { diff --git a/packages/core/test/acceptance/providers_spec.ts b/packages/core/test/acceptance/providers_spec.ts index 2c9e295724..0c46b74098 100644 --- a/packages/core/test/acceptance/providers_spec.ts +++ b/packages/core/test/acceptance/providers_spec.ts @@ -467,29 +467,29 @@ describe('providers', () => { }); - onlyInIvy('VE bug (see FW-1454)') - .it('should support forward refs in useClass when token is provided', () => { + it('should support forward refs in useClass when token is provided', () => { + @Injectable({providedIn: 'root'}) + abstract class SomeProvider { + } - @Injectable({providedIn: 'root', useClass: forwardRef(() => SomeProviderImpl)}) - abstract class SomeProvider { - } + @Injectable() + class SomeProviderImpl extends SomeProvider { + } - @Injectable() - class SomeProviderImpl extends SomeProvider { - } + @Component({selector: 'my-app', template: ''}) + class App { + constructor(public foo: SomeProvider) {} + } - @Component({selector: 'my-app', template: ''}) - class App { - constructor(public foo: SomeProvider) {} - } + TestBed.configureTestingModule({ + declarations: [App], + providers: [{provide: SomeProvider, useClass: forwardRef(() => SomeProviderImpl)}] + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); - TestBed.configureTestingModule( - {declarations: [App], providers: [{provide: SomeProvider, useClass: SomeProvider}]}); - const fixture = TestBed.createComponent(App); - fixture.detectChanges(); - - expect(fixture.componentInstance.foo).toBeAnInstanceOf(SomeProviderImpl); - }); + expect(fixture.componentInstance.foo).toBeAnInstanceOf(SomeProviderImpl); + }); });