From 8973f12ee4b1e04118ce2f1a6a48b93a6ff92c4d Mon Sep 17 00:00:00 2001 From: Marc Laval Date: Tue, 4 Dec 2018 14:34:43 +0100 Subject: [PATCH] fix(ivy): provided Injector should be instantiated by its factory (#27456) (FW-777) When an Injector is provided, R3Injector instantiates it by calling its constructor instead of its factory, not resolving dependencies. With this fix, the ngInjectorDef is checked and the factory is correctly used if it is found. PR Close #27456 --- packages/core/src/di/r3_injector.ts | 13 ++- .../hello_world_r2/bundle.golden_symbols.json | 2 +- .../injection/bundle.golden_symbols.json | 2 +- .../todo_r2/bundle.golden_symbols.json | 2 +- packages/core/test/di/r3_injector_spec.ts | 15 ++++ packages/router/upgrade/test/upgrade.spec.ts | 86 +++++++++---------- 6 files changed, 68 insertions(+), 52 deletions(-) diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index 0c679065263..715d1c76f27 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -166,7 +166,7 @@ export class R3Injector { if (def && this.injectableDefInScope(def)) { // Found an ngInjectableDef and it's scoped to this injector. Pretend as if it was here // all along. - record = makeRecord(injectableDefFactory(token), NOT_YET); + record = makeRecord(injectableDefOrInjectorDefFactory(token), NOT_YET); this.records.set(token, record); } } @@ -339,9 +339,14 @@ export class R3Injector { } } -function injectableDefFactory(token: Type| InjectionToken): () => any { +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; + } + if (token instanceof InjectionToken) { throw new Error(`Token ${stringify(token)} is missing an ngInjectableDef definition.`); } @@ -369,7 +374,7 @@ function providerToRecord(provider: SingleProvider): Record { export function providerToFactory(provider: SingleProvider): () => any { let factory: (() => any)|undefined = undefined; if (isTypeProvider(provider)) { - return injectableDefFactory(resolveForwardRef(provider)); + return injectableDefOrInjectorDefFactory(resolveForwardRef(provider)); } else { if (isValueProvider(provider)) { factory = () => resolveForwardRef(provider.useValue); @@ -383,7 +388,7 @@ export function providerToFactory(provider: SingleProvider): () => any { if (hasDeps(provider)) { factory = () => new (classRef)(...injectArgs(provider.deps)); } else { - return injectableDefFactory(classRef); + return injectableDefOrInjectorDefFactory(classRef); } } } diff --git a/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json index 17107811f94..6b29d92397a 100644 --- a/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json @@ -1008,7 +1008,7 @@ "name": "injectRootLimpMode" }, { - "name": "injectableDefFactory" + "name": "injectableDefOrInjectorDefFactory" }, { "name": "insertBloom" diff --git a/packages/core/test/bundling/injection/bundle.golden_symbols.json b/packages/core/test/bundling/injection/bundle.golden_symbols.json index 48f070ca93b..d3e45977b32 100644 --- a/packages/core/test/bundling/injection/bundle.golden_symbols.json +++ b/packages/core/test/bundling/injection/bundle.golden_symbols.json @@ -141,7 +141,7 @@ "name": "injectRootLimpMode" }, { - "name": "injectableDefFactory" + "name": "injectableDefOrInjectorDefFactory" }, { "name": "isExistingProvider" diff --git a/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json b/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json index d47ef9743ca..65f23d29947 100644 --- a/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json @@ -2082,7 +2082,7 @@ "name": "injectViewContainerRef" }, { - "name": "injectableDefFactory" + "name": "injectableDefOrInjectorDefFactory" }, { "name": "insertBloom" diff --git a/packages/core/test/di/r3_injector_spec.ts b/packages/core/test/di/r3_injector_spec.ts index 1c7e94505f5..421f32e5d1d 100644 --- a/packages/core/test/di/r3_injector_spec.ts +++ b/packages/core/test/di/r3_injector_spec.ts @@ -121,6 +121,14 @@ describe('InjectorDef-based createInjector()', () => { }); } + class InjectorWithDep { + constructor(readonly service: Service) {} + + static ngInjectorDef = defineInjector({ + factory: () => new InjectorWithDep(inject(Service)), + }); + } + class Module { static ngInjectorDef = defineInjector({ factory: () => new Module(), @@ -137,6 +145,7 @@ describe('InjectorDef-based createInjector()', () => { CircularA, CircularB, {provide: STATIC_TOKEN, useClass: StaticService, deps: [Service]}, + InjectorWithDep, ], }); } @@ -204,6 +213,12 @@ describe('InjectorDef-based createInjector()', () => { expect(instance.locale).toEqual(['en', 'es']); }); + it('injects an injector with dependencies', () => { + const instance = injector.get(InjectorWithDep); + expect(instance instanceof InjectorWithDep); + expect(instance.service).toBe(injector.get(Service)); + }); + it('injects a token with useExisting', () => { const instance = injector.get(SERVICE_TOKEN); expect(instance).toBe(injector.get(Service)); diff --git a/packages/router/upgrade/test/upgrade.spec.ts b/packages/router/upgrade/test/upgrade.spec.ts index 5a6015cbb31..d2ab3110127 100644 --- a/packages/router/upgrade/test/upgrade.spec.ts +++ b/packages/router/upgrade/test/upgrade.spec.ts @@ -44,63 +44,59 @@ describe('setUpLocationSync', () => { `); }); - fixmeIvy('FW-777: R3Injector doesn\'t support injectors as a provider') && - it('should get the $rootScope from AngularJS and set an $on watch on $locationChangeStart', - () => { - const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); + it('should get the $rootScope from AngularJS and set an $on watch on $locationChangeStart', + () => { + const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); - upgradeModule.$injector.get.and.callFake( - (name: string) => (name === '$rootScope') && $rootScope); + upgradeModule.$injector.get.and.callFake( + (name: string) => (name === '$rootScope') && $rootScope); - setUpLocationSync(upgradeModule); + setUpLocationSync(upgradeModule); - expect($rootScope.$on).toHaveBeenCalledTimes(1); - expect($rootScope.$on) - .toHaveBeenCalledWith('$locationChangeStart', jasmine.any(Function)); - }); + expect($rootScope.$on).toHaveBeenCalledTimes(1); + expect($rootScope.$on).toHaveBeenCalledWith('$locationChangeStart', jasmine.any(Function)); + }); - fixmeIvy('FW-777: R3Injector doesn\'t support injectors as a provider') && - it('should navigate by url every time $locationChangeStart is broadcasted', () => { - const url = 'https://google.com'; - const pathname = '/custom/route'; - const normalizedPathname = 'foo'; - const query = '?query=1&query2=3'; - const hash = '#new/hash'; - const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); + it('should navigate by url every time $locationChangeStart is broadcasted', () => { + const url = 'https://google.com'; + const pathname = '/custom/route'; + const normalizedPathname = 'foo'; + const query = '?query=1&query2=3'; + const hash = '#new/hash'; + const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); - upgradeModule.$injector.get.and.returnValue($rootScope); - LocationMock.normalize.and.returnValue(normalizedPathname); + upgradeModule.$injector.get.and.returnValue($rootScope); + LocationMock.normalize.and.returnValue(normalizedPathname); - setUpLocationSync(upgradeModule); + setUpLocationSync(upgradeModule); - const callback = $rootScope.$on.calls.argsFor(0)[1]; - callback({}, url + pathname + query + hash, ''); + const callback = $rootScope.$on.calls.argsFor(0)[1]; + callback({}, url + pathname + query + hash, ''); - expect(LocationMock.normalize).toHaveBeenCalledTimes(1); - expect(LocationMock.normalize).toHaveBeenCalledWith(pathname); + expect(LocationMock.normalize).toHaveBeenCalledTimes(1); + expect(LocationMock.normalize).toHaveBeenCalledWith(pathname); - expect(RouterMock.navigateByUrl).toHaveBeenCalledTimes(1); - expect(RouterMock.navigateByUrl).toHaveBeenCalledWith(normalizedPathname + query + hash); - }); + expect(RouterMock.navigateByUrl).toHaveBeenCalledTimes(1); + expect(RouterMock.navigateByUrl).toHaveBeenCalledWith(normalizedPathname + query + hash); + }); - fixmeIvy('FW-777: R3Injector doesn\'t support injectors as a provider') && - it('should work correctly on browsers that do not start pathname with `/`', () => { - const anchorProto = HTMLAnchorElement.prototype; - const originalDescriptor = Object.getOwnPropertyDescriptor(anchorProto, 'pathname'); - Object.defineProperty(anchorProto, 'pathname', {get: () => 'foo/bar'}); + it('should work correctly on browsers that do not start pathname with `/`', () => { + const anchorProto = HTMLAnchorElement.prototype; + const originalDescriptor = Object.getOwnPropertyDescriptor(anchorProto, 'pathname'); + Object.defineProperty(anchorProto, 'pathname', {get: () => 'foo/bar'}); - try { - const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); - upgradeModule.$injector.get.and.returnValue($rootScope); + try { + const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); + upgradeModule.$injector.get.and.returnValue($rootScope); - setUpLocationSync(upgradeModule); + setUpLocationSync(upgradeModule); - const callback = $rootScope.$on.calls.argsFor(0)[1]; - callback({}, '', ''); + const callback = $rootScope.$on.calls.argsFor(0)[1]; + callback({}, '', ''); - expect(LocationMock.normalize).toHaveBeenCalledWith('/foo/bar'); - } finally { - Object.defineProperty(anchorProto, 'pathname', originalDescriptor !); - } - }); + expect(LocationMock.normalize).toHaveBeenCalledWith('/foo/bar'); + } finally { + Object.defineProperty(anchorProto, 'pathname', originalDescriptor !); + } + }); });