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

@@ -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<any>|InjectorTypeWithProviders<any>,
-      parents: InjectorType<any>[], dedupStack: InjectorType<any>[]) {
+      parents: InjectorType<any>[],
+      dedupStack: InjectorType<any>[]): defOrWrappedDef is InjectorTypeWithProviders<any> {
     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<any>).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<any>[])|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<any>).ngModule;
-    deepForEach(providers, provider => this.processProvider(provider, ngModuleType, providers));
+    return (
+        ngModule !== undefined &&
+        (defOrWrappedDef as InjectorTypeWithProviders<any>).providers !== undefined);
   }
 
   /**

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",
     ],

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<string[]>('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[] = [];

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',
+    ]);
+  });
+});

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);