From 1a0e500eea811617193369add6007e878a9c36a7 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Mon, 13 May 2019 19:43:11 +0100 Subject: [PATCH] fix(ivy): support sub-class services that only inherit `@Injectable` (#30388) In View engine it is possible to instantiate a service that that has no `@Injectable` decorator as long as it satisfies one of: 1) It has no dependencies and so a constructor with no parameters. This is already supported in Ivy. 2) It has no constructor of its own and sub-classes a service which has dependencies but has its own `@Injectable` decorator. This second scenario was broken in Ivy. In Ivy, previous to this commit, if a class to be instantiated did not have its own `@Injectable` decorator and did not provide a constructor of its own, then it would be created using `new` with no arguments - i.e. falling back to the first scenario. After this commit Ivy correctly uses the `ngInjectableDef` inherited from the super-class to provide the `factory` for instantiating the sub-class. FW-1314 PR Close #30388 --- packages/core/src/di/interface/defs.ts | 28 +++++++- packages/core/src/di/r3_injector.ts | 65 +++++++++++++------ packages/core/test/acceptance/di_spec.ts | 38 ++++++++++- .../injection/bundle.golden_symbols.json | 6 ++ 4 files changed, 113 insertions(+), 24 deletions(-) diff --git a/packages/core/src/di/interface/defs.ts b/packages/core/src/di/interface/defs.ts index 041e599e08..f98c09cf6a 100644 --- a/packages/core/src/di/interface/defs.ts +++ b/packages/core/src/di/interface/defs.ts @@ -182,12 +182,34 @@ export function ΔdefineInjector(options: {factory: () => any, providers?: any[] } /** - * Read the `ngInjectableDef` type in a way which is immune to accidentally reading inherited value. + * Read the `ngInjectableDef` for `type` in a way which is immune to accidentally reading inherited + * value. * - * @param type type which may have `ngInjectableDef` + * @param type A type which may have its own (non-inherited) `ngInjectableDef`. */ export function getInjectableDef(type: any): ΔInjectableDef|null { - return type && type.hasOwnProperty(NG_INJECTABLE_DEF) ? (type as any)[NG_INJECTABLE_DEF] : null; + return type && type.hasOwnProperty(NG_INJECTABLE_DEF) ? type[NG_INJECTABLE_DEF] : null; +} + +/** + * Read the `ngInjectableDef` for `type` or read the `ngInjectableDef` from one of its ancestors. + * + * @param type A type which may have `ngInjectableDef`, via inheritance. + * + * @deprecated Will be removed in v10, where an error will occur in the scenario if we find the + * `ngInjectableDef` on an ancestor only. + */ +export function getInheritedInjectableDef(type: any): ΔInjectableDef|null { + if (type && type[NG_INJECTABLE_DEF]) { + // TODO(FW-1307): Re-add ngDevMode when closure can handle it + // ngDevMode && + console.warn( + `DEPRECATED: DI is instantiating a token "${type.name}" that inherits its @Injectable decorator but does not provide one itself.\n` + + `This will become an error in v10. Please add @Injectable() to the "${type.name}" class.`); + return type[NG_INJECTABLE_DEF]; + } else { + return null; + } } /** diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index 5a61c08c88..d7c2daca54 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -15,7 +15,7 @@ import {resolveForwardRef} from './forward_ref'; import {InjectionToken} from './injection_token'; import {Injector} from './injector'; import {INJECTOR, NG_TEMP_TOKEN_PATH, NullInjector, THROW_IF_NOT_FOUND, USE_VALUE, catchInjectorError, injectArgs, setCurrentInjector, Δinject} from './injector_compatibility'; -import {InjectableType, InjectorType, InjectorTypeWithProviders, getInjectableDef, getInjectorDef, ΔInjectableDef} from './interface/defs'; +import {InjectorType, InjectorTypeWithProviders, getInheritedInjectableDef, getInjectableDef, getInjectorDef, ΔInjectableDef} from './interface/defs'; import {InjectFlags} from './interface/injector'; import {ClassProvider, ConstructorProvider, ExistingProvider, FactoryProvider, StaticClassProvider, StaticProvider, TypeProvider, ValueProvider} from './interface/provider'; import {APP_ROOT} from './scope'; @@ -378,25 +378,52 @@ export class R3Injector { } function injectableDefOrInjectorDefFactory(token: Type| InjectionToken): () => any { - const injectableDef = getInjectableDef(token as InjectableType); - if (injectableDef === null) { - const injectorDef = getInjectorDef(token as InjectorType); - if (injectorDef !== null) { - return injectorDef.factory; - } else if (token instanceof InjectionToken) { - throw new Error(`Token ${stringify(token)} is missing an ngInjectableDef definition.`); - } else if (token instanceof Function) { - const paramLength = token.length; - if (paramLength > 0) { - const args: string[] = new Array(paramLength).fill('?'); - throw new Error( - `Can't resolve all parameters for ${stringify(token)}: (${args.join(', ')}).`); - } - return () => new (token as Type)(); - } - throw new Error('unreachable'); + // Most tokens will have an ngInjectableDef directly on them, which specifies a factory directly. + const injectableDef = getInjectableDef(token); + if (injectableDef !== null) { + return injectableDef.factory; + } + + // If the token is an NgModule, it's also injectable but the factory is on its ngInjectorDef. + const injectorDef = getInjectorDef(token); + if (injectorDef !== null) { + return injectorDef.factory; + } + + // InjectionTokens should have an ngInjectableDef and thus should be handled above. + // If it's missing that, it's an error. + if (token instanceof InjectionToken) { + throw new Error(`Token ${stringify(token)} is missing an ngInjectableDef definition.`); + } + + // Undecorated types can sometimes be created if they have no constructor arguments. + if (token instanceof Function) { + return getUndecoratedInjectableFactory(token); + } + + // There was no way to resolve a factory for this token. + throw new Error('unreachable'); +} + +function getUndecoratedInjectableFactory(token: Function) { + // If the token has parameters then it has dependencies that we cannot resolve implicitly. + const paramLength = token.length; + if (paramLength > 0) { + const args: string[] = new Array(paramLength).fill('?'); + throw new Error(`Can't resolve all parameters for ${stringify(token)}: (${args.join(', ')}).`); + } + + // The constructor function appears to have no parameters. + // This might be because it inherits from a super-class. In which case, use an ngInjectableDef + // from an ancestor if there is one. + // Otherwise this really is a simple class with no dependencies, so return a factory that + // just instantiates the zero-arg constructor. + const inheritedInjectableDef = getInheritedInjectableDef(token); + if (inheritedInjectableDef !== null) { + return inheritedInjectableDef.factory; + } else { + return () => new (token as Type)(); } - return injectableDef.factory; } function providerToRecord( diff --git a/packages/core/test/acceptance/di_spec.ts b/packages/core/test/acceptance/di_spec.ts index f0baec7bc8..a60bf34bbb 100644 --- a/packages/core/test/acceptance/di_spec.ts +++ b/packages/core/test/acceptance/di_spec.ts @@ -10,7 +10,7 @@ import {CommonModule} from '@angular/common'; import {Attribute, ChangeDetectorRef, Component, Directive, ElementRef, EventEmitter, Host, HostBinding, INJECTOR, Inject, Injectable, Injector, Input, LOCALE_ID, Optional, Output, Pipe, PipeTransform, Self, SkipSelf, TemplateRef, ViewChild, ViewContainerRef, forwardRef} from '@angular/core'; import {ViewRef} from '@angular/core/src/render3/view_ref'; import {TestBed} from '@angular/core/testing'; -import {onlyInIvy} from '@angular/private/testing'; +import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; describe('di', () => { describe('no dependencies', () => { @@ -370,7 +370,7 @@ describe('di', () => {
{{ dir.dirB.value }}
- +
@@ -858,6 +858,40 @@ describe('di', () => { const divElement = fixture.nativeElement.querySelector('div'); expect(divElement.textContent).toEqual('MyService'); }); + + it('should support sub-classes with no @Injectable decorator', () => { + @Injectable() + class Dependency { + } + + @Injectable() + class SuperClass { + constructor(public dep: Dependency) {} + } + + // Note, no @Injectable decorators for these two classes + class SubClass extends SuperClass {} + class SubSubClass extends SubClass {} + + @Component({template: ''}) + class MyComp { + constructor(public myService: SuperClass) {} + } + TestBed.configureTestingModule({ + declarations: [MyComp], + providers: [{provide: SuperClass, useClass: SubSubClass}, Dependency] + }); + + const warnSpy = spyOn(console, 'warn'); + const fixture = TestBed.createComponent(MyComp); + expect(fixture.componentInstance.myService.dep instanceof Dependency).toBe(true); + + if (ivyEnabled) { + expect(warnSpy).toHaveBeenCalledWith( + `DEPRECATED: DI is instantiating a token "SubSubClass" that inherits its @Injectable decorator but does not provide one itself.\n` + + `This will become an error in v10. Please add @Injectable() to the "SubSubClass" class.`); + } + }); }); describe('inject', () => { diff --git a/packages/core/test/bundling/injection/bundle.golden_symbols.json b/packages/core/test/bundling/injection/bundle.golden_symbols.json index 5e59706419..50cbf4564b 100644 --- a/packages/core/test/bundling/injection/bundle.golden_symbols.json +++ b/packages/core/test/bundling/injection/bundle.golden_symbols.json @@ -122,6 +122,9 @@ { "name": "getGlobal" }, + { + "name": "getInheritedInjectableDef" + }, { "name": "getInjectableDef" }, @@ -131,6 +134,9 @@ { "name": "getNullInjector" }, + { + "name": "getUndecoratedInjectableFactory" + }, { "name": "hasDeps" },