fix(ngcc): ensure private exports are added for `ModuleWithProviders` (#32902)

ngcc may need to insert public exports into the bundle's source as well
as to the entry-point's declaration file, as the Ivy compiler may need
to create import statements to internal library types. The way ngcc
knows which exports to add is through the references registry, to which
references to things that require a public export are added by the
various analysis steps that are executed.

One of these analysis steps is the augmentation of declaration files
where functions that return `ModuleWithProviders` are updated so that a
generic type argument is added that corresponds with the `NgModule` that
is actually imported. This type has to be publicly exported, so the
analyzer step has to add the module type to the references registry.

A problem occurs when `ModuleWithProviders` already has a generic type
argument, in which case no update of the declaration file is necessary.
This may happen when 1) ngcc is processing additional bundle formats, so
that the declaration file has already been updated while processing the
first bundle format, or 2) when a package is processed which already
contains the generic type in its source. In both scenarios it may occur
that the referenced `NgModule` type does not yet have a public export,
so it is crucial that a reference to the type is added to the
references registry, which ngcc failed to do.

This commit fixes the issue by always adding the referenced `NgModule`
type to the references registry, so that a public export will always be
created if necessary.

Resolves FW-1575

PR Close #32902

packages/compiler-cli/ngcc/src/analysis/module_with_providers_analyzer.ts

@@ -38,30 +38,17 @@ export class ModuleWithProvidersAnalyzer {
     rootFiles.forEach(f => {
       const fns = this.host.getModuleWithProvidersFunctions(f);
       fns && fns.forEach(fn => {
+        if (fn.ngModule.viaModule === null) {
+          // Record the usage of an internal module as it needs to become an exported symbol
+          this.referencesRegistry.add(fn.ngModule.node, new Reference(fn.ngModule.node));
+        }
+
         const dtsFn = this.getDtsDeclarationForFunction(fn);
         const typeParam = dtsFn.type && ts.isTypeReferenceNode(dtsFn.type) &&
                 dtsFn.type.typeArguments && dtsFn.type.typeArguments[0] ||
             null;
         if (!typeParam || isAnyKeyword(typeParam)) {
-          // Either we do not have a parameterized type or the type is `any`.
+          const ngModule = this.resolveNgModuleReference(fn);
-          let ngModule = fn.ngModule;
-          // For internal (non-library) module references, redirect the module's value declaration
-          // to its type declaration.
-          if (ngModule.viaModule === null) {
-            const dtsNgModule = this.host.getDtsDeclaration(ngModule.node);
-            if (!dtsNgModule) {
-              throw new Error(
-                  `No typings declaration can be found for the referenced NgModule class in ${fn.declaration.getText()}.`);
-            }
-            if (!ts.isClassDeclaration(dtsNgModule) || !hasNameIdentifier(dtsNgModule)) {
-              throw new Error(
-                  `The referenced NgModule in ${fn.declaration.getText()} is not a named class declaration in the typings program; instead we get ${dtsNgModule.getText()}`);
-            }
-            // Record the usage of the internal module as it needs to become an exported symbol
-            this.referencesRegistry.add(ngModule.node, new Reference(ngModule.node));
-
-            ngModule = {node: dtsNgModule, viaModule: null};
-          }
           const dtsFile = dtsFn.getSourceFile();
           const analysis = analyses.has(dtsFile) ? analyses.get(dtsFile) : [];
           analysis.push({declaration: dtsFn, ngModule});
@@ -100,6 +87,30 @@ export class ModuleWithProvidersAnalyzer {
     }
     return dtsFn;
   }
+
+  private resolveNgModuleReference(fn: ModuleWithProvidersFunction):
+      ConcreteDeclaration<ClassDeclaration> {
+    const ngModule = fn.ngModule;
+
+    // For external module references, use the declaration as is.
+    if (ngModule.viaModule !== null) {
+      return ngModule;
+    }
+
+    // For internal (non-library) module references, redirect the module's value declaration
+    // to its type declaration.
+    const dtsNgModule = this.host.getDtsDeclaration(ngModule.node);
+    if (!dtsNgModule) {
+      throw new Error(
+          `No typings declaration can be found for the referenced NgModule class in ${fn.declaration.getText()}.`);
+    }
+    if (!ts.isClassDeclaration(dtsNgModule) || !hasNameIdentifier(dtsNgModule)) {
+      throw new Error(
+          `The referenced NgModule in ${fn.declaration.getText()} is not a named class declaration in the typings program; instead we get ${dtsNgModule.getText()}`);
+    }
+
+    return {node: dtsNgModule, viaModule: null};
+  }
 }
 
 

packages/compiler-cli/ngcc/test/analysis/module_with_providers_analyzer_spec.ts

@@ -24,7 +24,7 @@ runInEachFileSystem(() => {
       let _: typeof absoluteFrom;
       let analyses: ModuleWithProvidersAnalyses;
       let program: ts.Program;
-      let dtsProgram: BundleProgram|null;
+      let dtsProgram: BundleProgram;
       let referencesRegistry: NgccReferencesRegistry;
 
       beforeEach(() => {
@@ -333,7 +333,7 @@ runInEachFileSystem(() => {
             'test-package', 'esm2015', false, getRootFiles(TEST_PROGRAM),
             getRootFiles(TEST_DTS_PROGRAM));
         program = bundle.src.program;
-        dtsProgram = bundle.dts;
+        dtsProgram = bundle.dts !;
         const host = new Esm2015ReflectionHost(
             new MockLogger(), false, program.getTypeChecker(), dtsProgram);
         referencesRegistry = new NgccReferencesRegistry(host);
@@ -412,7 +412,7 @@ runInEachFileSystem(() => {
 
       function getAnalysisDescription(
           analyses: ModuleWithProvidersAnalyses, fileName: AbsoluteFsPath) {
-        const file = getSourceFileOrError(dtsProgram !.program, fileName);
+        const file = getSourceFileOrError(dtsProgram.program, fileName);
         const analysis = analyses.get(file);
         return analysis ?
             analysis.map(
@@ -424,4 +424,144 @@ runInEachFileSystem(() => {
       }
     });
   });
+
+  describe('tracking references when generic types already present', () => {
+    let _: typeof absoluteFrom;
+    let analyses: ModuleWithProvidersAnalyses;
+    let program: ts.Program;
+    let dtsProgram: BundleProgram;
+    let referencesRegistry: NgccReferencesRegistry;
+
+    beforeEach(() => {
+      _ = absoluteFrom;
+
+      const TEST_PROGRAM: TestFile[] = [
+        {
+          name: _('/node_modules/test-package/src/entry-point.js'),
+          contents: `
+          export * from './explicit';
+          export * from './module';
+        `
+        },
+        {
+          name: _('/node_modules/test-package/src/explicit.js'),
+          contents: `
+          import {ExternalModule} from './module';
+          import {LibraryModule} from 'some-library';
+          export class ExplicitInternalModule {}
+          export class ExplicitClass {
+            static explicitInternalMethod() {
+              return {
+                ngModule: ExplicitInternalModule,
+                providers: []
+              };
+            }
+            static explicitExternalMethod() {
+              return {
+                ngModule: ExternalModule,
+                providers: []
+              };
+            }
+            static explicitLibraryMethod() {
+              return {
+                ngModule: LibraryModule,
+                providers: []
+              };
+            }
+          }
+          `
+        },
+        {
+          name: _('/node_modules/test-package/src/module.js'),
+          contents: `
+          export class ExternalModule {}
+          `
+        },
+        {
+          name: _('/node_modules/some-library/index.d.ts'),
+          contents: 'export declare class LibraryModule {}'
+        },
+      ];
+      const TEST_DTS_PROGRAM: TestFile[] = [
+        {
+          name: _('/node_modules/test-package/typings/entry-point.d.ts'),
+          contents: `
+          export * from './explicit';
+          export * from './module';
+        `
+        },
+        {
+          name: _('/node_modules/test-package/typings/explicit.d.ts'),
+          contents: `
+          import {ModuleWithProviders} from './core';
+          import {ExternalModule} from './module';
+          import {LibraryModule} from 'some-library';
+          export declare class ExplicitInternalModule {}
+          export declare class ExplicitClass {
+            static explicitInternalMethod(): ModuleWithProviders<ExplicitInternalModule>;
+            static explicitExternalMethod(): ModuleWithProviders<ExternalModule>;
+            static explicitLibraryMethod(): ModuleWithProviders<LibraryModule>;
+          }
+          `
+        },
+        {
+          name: _('/node_modules/test-package/typings/module.d.ts'),
+          contents: `
+          export declare class ExternalModule {}
+          `
+        },
+        {
+          name: _('/node_modules/test-package/typings/core.d.ts'),
+          contents: `
+
+          export declare interface Type<T> {
+            new (...args: any[]): T
+          }
+          export declare type Provider = any;
+          export declare interface ModuleWithProviders<T> {
+            ngModule: Type<T>
+            providers?: Provider[]
+          }
+        `
+        },
+        {
+          name: _('/node_modules/some-library/index.d.ts'),
+          contents: 'export declare class LibraryModule {}'
+        },
+      ];
+      loadTestFiles(TEST_PROGRAM);
+      loadTestFiles(TEST_DTS_PROGRAM);
+      const bundle = makeTestEntryPointBundle(
+          'test-package', 'esm2015', false, getRootFiles(TEST_PROGRAM),
+          getRootFiles(TEST_DTS_PROGRAM));
+      program = bundle.src.program;
+      dtsProgram = bundle.dts !;
+      const host =
+          new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dtsProgram);
+      referencesRegistry = new NgccReferencesRegistry(host);
+
+      const analyzer = new ModuleWithProvidersAnalyzer(host, referencesRegistry);
+      analyses = analyzer.analyzeProgram(program);
+    });
+
+    it('should track references even when nothing needs to be updated', () => {
+      const file = getSourceFileOrError(
+          dtsProgram.program, _('/node_modules/test-package/typings/explicit.d.ts'));
+      expect(analyses.has(file)).toBe(false);
+
+      const declarations = referencesRegistry.getDeclarationMap();
+      const explicitInternalModuleDeclaration = getDeclaration(
+          program, absoluteFrom('/node_modules/test-package/src/explicit.js'),
+          'ExplicitInternalModule', ts.isClassDeclaration);
+      const externalModuleDeclaration = getDeclaration(
+          program, absoluteFrom('/node_modules/test-package/src/module.js'), 'ExternalModule',
+          ts.isClassDeclaration);
+      const libraryModuleDeclaration = getDeclaration(
+          program, absoluteFrom('/node_modules/some-library/index.d.ts'), 'LibraryModule',
+          ts.isClassDeclaration);
+      expect(declarations.has(explicitInternalModuleDeclaration.name !)).toBe(true);
+      expect(declarations.has(externalModuleDeclaration.name !)).toBe(true);
+      expect(declarations.has(libraryModuleDeclaration.name !)).toBe(false);
+    });
+  });
 });