From 6d861f240b4a601fcdb4a2e76ac561a7f0c54875 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 27 May 2019 20:34:01 +0200 Subject: [PATCH] fix(ivy): module with providers are processed too early (#30688) Currently with Ivy, `ModuleWithProvider` providers are processed in order of declaration in the `NgModule` imports. This technically makes makes sense but is a potential breaking change as `ModuleWithProvider` providers are processed after all imported modules in View Engine. In order to keep the behavior of View Engine, the `r3_injector` is updated to no longer process `ModuleWithProvider` providers egarly. Resolves FW-1349 PR Close #30688 --- packages/core/src/di/r3_injector.ts | 49 +++++++++----- packages/core/test/acceptance/BUILD.bazel | 1 + packages/core/test/acceptance/di_spec.ts | 28 +++++++- .../acceptance/router_integration_spec.ts | 64 +++++++++++++++++++ packages/core/test/di/r3_injector_spec.ts | 30 +++++++++ 5 files changed, 156 insertions(+), 16 deletions(-) create mode 100644 packages/core/test/acceptance/router_integration_spec.ts diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index be8b0cb07c..b5863856df 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -222,14 +222,20 @@ export class R3Injector { } /** - * Add an `InjectorType` or `InjectorDefTypeWithProviders` and all of its transitive providers + * Add an `InjectorType` or `InjectorTypeWithProviders` and all of its transitive providers * to this injector. + * + * If an `InjectorTypeWithProviders` that declares providers besides the type is specified, + * the function will return "true" to indicate that the providers of the type definition need + * to be processed. This allows us to process providers of injector types after all imports of + * an injector definition are processed. (following View Engine semantics: see FW-1349) */ private processInjectorType( defOrWrappedDef: InjectorType|InjectorTypeWithProviders, - parents: InjectorType[], dedupStack: InjectorType[]) { + parents: InjectorType[], + dedupStack: InjectorType[]): defOrWrappedDef is InjectorTypeWithProviders { defOrWrappedDef = resolveForwardRef(defOrWrappedDef); - if (!defOrWrappedDef) return; + if (!defOrWrappedDef) return false; // Either the defOrWrappedDef is an InjectorType (with ngInjectorDef) or an // InjectorDefTypeWithProviders (aka ModuleWithProviders). Detecting either is a megamorphic @@ -259,12 +265,6 @@ export class R3Injector { // Check for multiple imports of the same module const isDuplicate = dedupStack.indexOf(defType) !== -1; - // If defOrWrappedType was an InjectorDefTypeWithProviders, then .providers may hold some - // extra providers. - const providers = - (ngModule !== undefined) && (defOrWrappedDef as InjectorTypeWithProviders).providers || - EMPTY_ARRAY; - // Finally, if defOrWrappedType was an `InjectorDefTypeWithProviders`, then the actual // `InjectorDef` is on its `ngModule`. if (ngModule !== undefined) { @@ -273,7 +273,7 @@ export class R3Injector { // If no definition was found, it might be from exports. Remove it. if (def == null) { - return; + return false; } // Track the InjectorType and add a provider for it. @@ -291,14 +291,33 @@ export class R3Injector { // Add it to the set of dedups. This way we can detect multiple imports of the same module dedupStack.push(defType); + let importTypesWithProviders: (InjectorTypeWithProviders[])|undefined; try { - deepForEach( - def.imports, imported => this.processInjectorType(imported, parents, dedupStack)); + deepForEach(def.imports, imported => { + if (this.processInjectorType(imported, parents, dedupStack)) { + if (importTypesWithProviders === undefined) importTypesWithProviders = []; + // If the processed import is an injector type with providers, we store it in the + // list of import types with providers, so that we can process those afterwards. + importTypesWithProviders.push(imported); + } + }); } finally { // Remove it from the parents set when finished. // TODO(FW-1307): Re-add ngDevMode when closure can handle it parents.pop(); } + + // Imports which are declared with providers (TypeWithProviders) need to be processed + // after all imported modules are processed. This is similar to how View Engine + // processes/merges module imports in the metadata resolver. See: FW-1349. + if (importTypesWithProviders !== undefined) { + for (let i = 0; i < importTypesWithProviders.length; i++) { + const {ngModule, providers} = importTypesWithProviders[i]; + deepForEach( + providers !, + provider => this.processProvider(provider, ngModule, providers || EMPTY_ARRAY)); + } + } } // Next, include providers listed on the definition itself. @@ -309,9 +328,9 @@ export class R3Injector { defProviders, provider => this.processProvider(provider, injectorType, defProviders)); } - // Finally, include providers from an InjectorDefTypeWithProviders if there was one. - const ngModuleType = (defOrWrappedDef as InjectorTypeWithProviders).ngModule; - deepForEach(providers, provider => this.processProvider(provider, ngModuleType, providers)); + return ( + ngModule !== undefined && + (defOrWrappedDef as InjectorTypeWithProviders).providers !== undefined); } /** diff --git a/packages/core/test/acceptance/BUILD.bazel b/packages/core/test/acceptance/BUILD.bazel index 1b7f6fd370..e87138061b 100644 --- a/packages/core/test/acceptance/BUILD.bazel +++ b/packages/core/test/acceptance/BUILD.bazel @@ -25,6 +25,7 @@ ts_library( "//packages/platform-browser/testing", "//packages/platform-server", "//packages/private/testing", + "//packages/router", "@npm//rxjs", "@npm//zone.js", ], diff --git a/packages/core/test/acceptance/di_spec.ts b/packages/core/test/acceptance/di_spec.ts index a624e478ac..cdb9c2bc63 100644 --- a/packages/core/test/acceptance/di_spec.ts +++ b/packages/core/test/acceptance/di_spec.ts @@ -7,7 +7,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 {Attribute, ChangeDetectorRef, Component, Directive, ElementRef, EventEmitter, Host, HostBinding, INJECTOR, Inject, Injectable, InjectionToken, Injector, Input, LOCALE_ID, ModuleWithProviders, NgModule, 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 {ivyEnabled, onlyInIvy} from '@angular/private/testing'; @@ -31,6 +31,32 @@ describe('di', () => { }); }); + describe('multi providers', () => { + it('should process ModuleWithProvider providers after module imports', () => { + const testToken = new InjectionToken('test-multi'); + + @NgModule({providers: [{provide: testToken, useValue: 'A', multi: true}]}) + class TestModuleA { + } + + @NgModule({providers: [{provide: testToken, useValue: 'B', multi: true}]}) + class TestModuleB { + } + + TestBed.configureTestingModule({ + imports: [ + { + ngModule: TestModuleA, + providers: [{provide: testToken, useValue: 'C', multi: true}], + }, + TestModuleB, + ] + }); + + expect(TestBed.get(testToken) as string[]).toEqual(['A', 'B', 'C']); + }); + }); + describe('directive injection', () => { let log: string[] = []; diff --git a/packages/core/test/acceptance/router_integration_spec.ts b/packages/core/test/acceptance/router_integration_spec.ts new file mode 100644 index 0000000000..bfa5666f2e --- /dev/null +++ b/packages/core/test/acceptance/router_integration_spec.ts @@ -0,0 +1,64 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {APP_BASE_HREF} from '@angular/common'; +import {NgModule} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import {Router, RouterModule} from '@angular/router'; + +describe('router integration acceptance', () => { + // Test case that ensures that we don't regress in multi-provider ordering + // which is leveraged in the router. See: FW-1349 + it('should have correct order for multiple routes declared in different modules', () => { + @NgModule({ + imports: [ + RouterModule.forChild([ + {path: '1a:1', redirectTo: ''}, + {path: '1a:2', redirectTo: ''}, + ]), + ], + }) + class Level1AModule { + } + + @NgModule({ + imports: [ + RouterModule.forChild([ + {path: '1b:1', redirectTo: ''}, + {path: '1b:2', redirectTo: ''}, + ]), + ], + }) + class Level1BModule { + } + + @NgModule({ + imports: [ + RouterModule.forRoot([{path: 'root', redirectTo: ''}]), + Level1AModule, + Level1BModule, + ], + providers: [ + {provide: APP_BASE_HREF, useValue: '/'}, + ] + }) + class RootModule { + } + + TestBed.configureTestingModule({ + imports: [RootModule], + }); + expect((TestBed.get(Router) as Router).config.map(r => r.path)).toEqual([ + '1a:1', + '1a:2', + '1b:1', + '1b:2', + 'root', + ]); + }); +}); diff --git a/packages/core/test/di/r3_injector_spec.ts b/packages/core/test/di/r3_injector_spec.ts index 75907a1aee..f60301a0f2 100644 --- a/packages/core/test/di/r3_injector_spec.ts +++ b/packages/core/test/di/r3_injector_spec.ts @@ -217,6 +217,31 @@ describe('InjectorDef-based createInjector()', () => { }); } + class MultiProviderA { + static ngInjectorDef = ɵɵdefineInjector({ + factory: () => new MultiProviderA(), + providers: [{provide: LOCALE, multi: true, useValue: 'A'}], + }); + } + + class MultiProviderB { + static ngInjectorDef = ɵɵdefineInjector({ + factory: () => new MultiProviderB(), + providers: [{provide: LOCALE, multi: true, useValue: 'B'}], + }); + } + + class WithProvidersTest { + static ngInjectorDef = ɵɵdefineInjector({ + factory: () => new WithProvidersTest(), + imports: [ + {ngModule: MultiProviderA, providers: [{provide: LOCALE, multi: true, useValue: 'C'}]}, + MultiProviderB + ], + providers: [], + }); + } + let injector: Injector; beforeEach(() => { @@ -274,6 +299,11 @@ describe('InjectorDef-based createInjector()', () => { expect(instance.locale).toEqual(['en', 'es']); }); + it('should process "InjectionTypeWithProviders" providers after imports injection type', () => { + injector = createInjector(WithProvidersTest); + expect(injector.get(LOCALE)).toEqual(['A', 'B', 'C']); + }); + it('injects an injector with dependencies', () => { const instance = injector.get(InjectorWithDep); expect(instance instanceof InjectorWithDep);