fix(ivy): deduplicate imported modules in r3_injector (#27102)

PR Close #27102

packages/compiler-cli/integrationtest/bazel/injector_def/ivy_build/app/test/module_spec.ts

@@ -54,7 +54,8 @@ describe('Ivy NgModule', () => {
       }
 
       expect(() => createInjector(AModule))
-          .toThrowError('Circular dependency: type AModule ends up importing itself.');
+          .toThrowError(
+              'Circular dependency in DI detected for type AModule. Dependency path: AModule > BModule > AModule.');
     });
 
     it('merges imports and exports', () => {

packages/core/src/di/r3_injector.ts

@@ -108,8 +108,8 @@ export class R3Injector {
       readonly parent: Injector) {
     // Start off by creating Records for every provider declared in every InjectorType
     // included transitively in `def`.
-    deepForEach(
+    const dedupStack: InjectorType<any>[] = [];
-        [def], injectorDef => this.processInjectorType(injectorDef, new Set<InjectorType<any>>()));
+    deepForEach([def], injectorDef => this.processInjectorType(injectorDef, [], dedupStack));
 
     additionalProviders &&
         deepForEach(additionalProviders, provider => this.processProvider(provider));
@@ -198,7 +198,7 @@ export class R3Injector {
    */
   private processInjectorType(
       defOrWrappedDef: InjectorType<any>|InjectorTypeWithProviders<any>,
-      parents: Set<InjectorType<any>>) {
+      parents: InjectorType<any>[], dedupStack: InjectorType<any>[]) {
     defOrWrappedDef = resolveForwardRef(defOrWrappedDef);
 
     // Either the defOrWrappedDef is an InjectorType (with ngInjectorDef) or an
@@ -218,6 +218,18 @@ export class R3Injector {
     const defType: InjectorType<any> =
         (ngModule === undefined) ? (defOrWrappedDef as InjectorType<any>) : ngModule;
 
+    // Check for circular dependencies.
+    if (ngDevMode && parents.indexOf(defType) !== -1) {
+      const defName = stringify(defType);
+      throw new Error(
+          `Circular dependency in DI detected for type ${defName}. Dependency path: ${parents.map(defType => stringify(defType)).join(' > ')} > ${defName}.`);
+    }
+
+    // Check for multiple imports of the same module
+    if (dedupStack.indexOf(defType) !== -1) {
+      return;
+    }
+
     // If defOrWrappedType was an InjectorDefTypeWithProviders, then .providers may hold some
     // extra providers.
     const providers =
@@ -235,11 +247,6 @@ export class R3Injector {
       return;
     }
 
-    // Check for circular dependencies.
-    if (parents.has(defType)) {
-      throw new Error(`Circular dependency: type ${stringify(defType)} ends up importing itself.`);
-    }
-
     // Track the InjectorType and add a provider for it.
     this.injectorDefTypes.add(defType);
     this.records.set(defType, makeRecord(def.factory));
@@ -250,12 +257,16 @@ export class R3Injector {
     if (def.imports != null) {
       // Before processing defType's imports, add it to the set of parents. This way, if it ends
       // up deeply importing itself, this can be detected.
-      parents.add(defType);
+      ngDevMode && parents.push(defType);
+      // Add it to the set of dedups. This way we can detect multiple imports of the same module
+      dedupStack.push(defType);
+
       try {
-        deepForEach(def.imports, imported => this.processInjectorType(imported, parents));
+        deepForEach(
+            def.imports, imported => this.processInjectorType(imported, parents, dedupStack));
       } finally {
         // Remove it from the parents set when finished.
-        parents.delete(defType);
+        ngDevMode && parents.pop();
       }
     }
 

packages/platform-server/test/integration_spec.ts

@@ -433,21 +433,20 @@ class HiddenModule {
          });
        }));
 
-    fixmeIvy('no deduplication of imported modules') &&
+    it('adds styles with ng-transition attribute', async(() => {
-        it('adds styles with ng-transition attribute', async(() => {
+         const platform = platformDynamicServer([{
-             const platform = platformDynamicServer([{
+           provide: INITIAL_CONFIG,
-               provide: INITIAL_CONFIG,
+           useValue: {document: '<html><head></head><body><app></app></body></html>'}
-               useValue: {document: '<html><head></head><body><app></app></body></html>'}
+         }]);
-             }]);
+         platform.bootstrapModule(ExampleStylesModule).then(ref => {
-             platform.bootstrapModule(ExampleStylesModule).then(ref => {
+           const doc = ref.injector.get(DOCUMENT);
-               const doc = ref.injector.get(DOCUMENT);
+           const head = getDOM().getElementsByTagName(doc, 'head')[0];
-               const head = getDOM().getElementsByTagName(doc, 'head')[0];
+           const styles: any[] = head.children as any;
-               const styles: any[] = head.children as any;
+           expect(styles.length).toBe(1);
-               expect(styles.length).toBe(1);
+           expect(getDOM().getText(styles[0])).toContain('color: red');
-               expect(getDOM().getText(styles[0])).toContain('color: red');
+           expect(getDOM().getAttribute(styles[0], 'ng-transition')).toBe('example-styles');
-               expect(getDOM().getAttribute(styles[0], 'ng-transition')).toBe('example-styles');
+         });
-             });
+       }));
-           }));
 
     it('copies known properties to attributes', async(() => {
          const platform = platformDynamicServer(
@@ -718,27 +717,25 @@ class HiddenModule {
            });
          }));
 
-      fixmeIvy('no deduplication of imported modules') &&
+      it('requests are macrotasks', async(() => {
-          it('requests are macrotasks', async(() => {
+           const platform = platformDynamicServer(
-               const platform = platformDynamicServer(
+               [{provide: INITIAL_CONFIG, useValue: {document: '<app></app>'}}]);
-                   [{provide: INITIAL_CONFIG, useValue: {document: '<app></app>'}}]);
+           platform.bootstrapModule(ExampleModule).then(ref => {
-               platform.bootstrapModule(ExampleModule).then(ref => {
+             const mock = ref.injector.get(MockBackend);
-                 const mock = ref.injector.get(MockBackend);
+             const http = ref.injector.get(Http);
-                 const http = ref.injector.get(Http);
+             expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeFalsy();
-                 expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeFalsy();
+             ref.injector.get<NgZone>(NgZone).run(() => {
-                 ref.injector.get<NgZone>(NgZone).run(() => {
+               NgZone.assertInAngularZone();
-                   NgZone.assertInAngularZone();
+               mock.connections.subscribe((mc: MockConnection) => {
-                   mock.connections.subscribe((mc: MockConnection) => {
+                 expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeTruthy();
-                     expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeTruthy();
+                 mc.mockRespond(new Response(new ResponseOptions({body: 'success!', status: 200})));
-                     mc.mockRespond(
-                         new Response(new ResponseOptions({body: 'success!', status: 200})));
-                   });
-                   http.get('http://localhost/testing').subscribe(resp => {
-                     expect(resp.text()).toBe('success!');
-                   });
-                 });
                });
-             }));
+               http.get('http://localhost/testing').subscribe(resp => {
+                 expect(resp.text()).toBe('success!');
+               });
+             });
+           });
+         }));
 
       it('works when HttpModule is included before ServerModule', async(() => {
            const platform = platformDynamicServer(
@@ -760,27 +757,25 @@ class HiddenModule {
            });
          }));
 
-      fixmeIvy('no deduplication of imported modules') &&
+      it('works when HttpModule is included after ServerModule', async(() => {
-          it('works when HttpModule is included after ServerModule', async(() => {
+           const platform = platformDynamicServer(
-               const platform = platformDynamicServer(
+               [{provide: INITIAL_CONFIG, useValue: {document: '<app></app>'}}]);
-                   [{provide: INITIAL_CONFIG, useValue: {document: '<app></app>'}}]);
+           platform.bootstrapModule(HttpAfterExampleModule).then(ref => {
-               platform.bootstrapModule(HttpAfterExampleModule).then(ref => {
+             const mock = ref.injector.get(MockBackend);
-                 const mock = ref.injector.get(MockBackend);
+             const http = ref.injector.get(Http);
-                 const http = ref.injector.get(Http);
+             expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeFalsy();
-                 expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeFalsy();
+             ref.injector.get<NgZone>(NgZone).run(() => {
-                 ref.injector.get<NgZone>(NgZone).run(() => {
+               NgZone.assertInAngularZone();
-                   NgZone.assertInAngularZone();
+               mock.connections.subscribe((mc: MockConnection) => {
-                   mock.connections.subscribe((mc: MockConnection) => {
+                 expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeTruthy();
-                     expect(ref.injector.get<NgZone>(NgZone).hasPendingMacrotasks).toBeTruthy();
+                 mc.mockRespond(new Response(new ResponseOptions({body: 'success!', status: 200})));
-                     mc.mockRespond(
-                         new Response(new ResponseOptions({body: 'success!', status: 200})));
-                   });
-                   http.get('http://localhost/testing').subscribe(resp => {
-                     expect(resp.text()).toBe('success!');
-                   });
-                 });
                });
-             }));
+               http.get('http://localhost/testing').subscribe(resp => {
+                 expect(resp.text()).toBe('success!');
+               });
+             });
+           });
+         }));
 
       it('throws when given a relative URL', async(() => {
            const platform = platformDynamicServer(