fix(ngcc): correctly detect dependencies in CommonJS (#34528)

Previously, `CommonJsDependencyHost.collectDependencies()` would only
find dependencies via imports of the form `var foo = require('...');` or
`var foo = require('...'), bar = require('...');` However, CommonJS
files can have imports in many different forms. By failing to recognize
other forms of imports, the associated dependencies were missed, which
in turn resulted in entry-points being compiled out-of-order and failing
due to that.

While we cannot easily capture all different types of imports, this
commit enhances `CommonJsDependencyHost` to recognize the following
common forms of imports:

- Imports in property assignments. E.g.:
  `exports.foo = require('...');` or
  `module.exports = {foo: require('...')};`

- Imports for side-effects only. E.g.:
  `require('...');`

- Star re-exports (with both emitted and imported heleprs). E.g.:
  `__export(require('...'));` or
  `tslib_1.__exportStar(require('...'), exports);`

PR Close #34528

packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts

@@ -7,7 +7,7 @@
  */
 import * as ts from 'typescript';
 import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
-import {isRequireCall} from '../host/commonjs_umd_utils';
+import {RequireCall, isReexportStatement, isRequireCall} from '../host/commonjs_umd_utils';
 import {DependencyHostBase} from './dependency_host';
 import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver';
 
@@ -40,33 +40,72 @@ export class CommonJsDependencyHost extends DependencyHostBase {
     // Parse the source into a TypeScript AST and then walk it looking for imports and re-exports.
     const sf =
         ts.createSourceFile(file, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS);
+    const requireCalls: RequireCall[] = [];
 
-    for (const statement of sf.statements) {
+    for (const stmt of sf.statements) {
-      const declarations =
+      if (ts.isVariableStatement(stmt)) {
-          ts.isVariableStatement(statement) ? statement.declarationList.declarations : [];
+        // Regular import(s):
-      for (const declaration of declarations) {
+        // `var foo = require('...')` or `var foo = require('...'), bar = require('...')`
-        if (declaration.initializer && isRequireCall(declaration.initializer)) {
+        const declarations = stmt.declarationList.declarations;
-          const importPath = declaration.initializer.arguments[0].text;
+        for (const declaration of declarations) {
-          const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file);
+          if ((declaration.initializer !== undefined) && isRequireCall(declaration.initializer)) {
-          if (resolvedModule) {
+            requireCalls.push(declaration.initializer);
-            if (resolvedModule instanceof ResolvedRelativeModule) {
-              const internalDependency = resolvedModule.modulePath;
-              if (!alreadySeen.has(internalDependency)) {
-                alreadySeen.add(internalDependency);
-                this.recursivelyCollectDependencies(
-                    internalDependency, dependencies, missing, deepImports, alreadySeen);
-              }
-            } else {
-              if (resolvedModule instanceof ResolvedDeepImport) {
-                deepImports.add(resolvedModule.importPath);
-              } else {
-                dependencies.add(resolvedModule.entryPointPath);
-              }
-            }
-          } else {
-            missing.add(importPath);
           }
         }
+      } else if (ts.isExpressionStatement(stmt)) {
+        if (isRequireCall(stmt.expression)) {
+          // Import for the side-effects only:
+          // `require('...')`
+          requireCalls.push(stmt.expression);
+        } else if (isReexportStatement(stmt)) {
+          // Re-export in one of the following formats:
+          // - `__export(require('...'))`
+          // - `__export(<identifier>)`
+          // - `tslib_1.__exportStar(require('...'), exports)`
+          // - `tslib_1.__exportStar(<identifier>, exports)`
+          const firstExportArg = stmt.expression.arguments[0];
+
+          if (isRequireCall(firstExportArg)) {
+            // Re-export with `require()` call:
+            // `__export(require('...'))` or `tslib_1.__exportStar(require('...'), exports)`
+            requireCalls.push(firstExportArg);
+          }
+        } else if (
+            ts.isBinaryExpression(stmt.expression) &&
+            (stmt.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken)) {
+          if (isRequireCall(stmt.expression.right)) {
+            // Import with assignment. E.g.:
+            // `exports.foo = require('...')`
+            requireCalls.push(stmt.expression.right);
+          } else if (ts.isObjectLiteralExpression(stmt.expression.right)) {
+            // Import in object literal. E.g.:
+            // `module.exports = {foo: require('...')}`
+            stmt.expression.right.properties.forEach(prop => {
+              if (ts.isPropertyAssignment(prop) && isRequireCall(prop.initializer)) {
+                requireCalls.push(prop.initializer);
+              }
+            });
+          }
+        }
+      }
+    }
+
+    const importPaths = new Set(requireCalls.map(call => call.arguments[0].text));
+    for (const importPath of importPaths) {
+      const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file);
+      if (resolvedModule === null) {
+        missing.add(importPath);
+      } else if (resolvedModule instanceof ResolvedRelativeModule) {
+        const internalDependency = resolvedModule.modulePath;
+        if (!alreadySeen.has(internalDependency)) {
+          alreadySeen.add(internalDependency);
+          this.recursivelyCollectDependencies(
+              internalDependency, dependencies, missing, deepImports, alreadySeen);
+        }
+      } else if (resolvedModule instanceof ResolvedDeepImport) {
+        deepImports.add(resolvedModule.importPath);
+      } else {
+        dependencies.add(resolvedModule.entryPointPath);
       }
     }
   }

packages/compiler-cli/ngcc/test/dependencies/commonjs_dependency_host_spec.ts

@@ -143,6 +143,142 @@ runInEachFileSystem(() => {
         expect(dependencies.has(_('/node_modules/lib_1/sub_1'))).toBe(true);
       });
 
+      it('should recognize imports in a variable declaration list', () => {
+        loadTestFiles([
+          {
+            name: _('/test/index.js'),
+            contents: commonJs({
+              varDeclarations: [
+                ['lib_1/sub_1', 'lib_1/sub_2'],
+              ],
+            }),
+          },
+          {name: _('/test/package.json'), contents: '{"main": "./index.js"}'},
+          {name: _('/test/index.metadata.json'), contents: 'MOCK METADATA'},
+        ]);
+
+        const {dependencies, missing, deepImports} = createDependencyInfo();
+        host.collectDependencies(_('/test/index.js'), {dependencies, missing, deepImports});
+
+        expect(dependencies.size).toBe(2);
+        expect(missing.size).toBe(0);
+        expect(deepImports.size).toBe(0);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_2'))).toBe(true);
+      });
+
+      it('should recognize imports as property assignments (on existing object)', () => {
+        loadTestFiles([
+          {
+            name: _('/test/index.js'),
+            contents: commonJs({
+              propAssignment: ['lib_1/sub_1', 'lib_1/sub_2'],
+            }),
+          },
+          {name: _('/test/package.json'), contents: '{"main": "./index.js"}'},
+          {name: _('/test/index.metadata.json'), contents: 'MOCK METADATA'},
+        ]);
+
+        const {dependencies, missing, deepImports} = createDependencyInfo();
+        host.collectDependencies(_('/test/index.js'), {dependencies, missing, deepImports});
+
+        expect(dependencies.size).toBe(2);
+        expect(missing.size).toBe(0);
+        expect(deepImports.size).toBe(0);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_2'))).toBe(true);
+      });
+
+      it('should recognize imports as property assignments (in object literal)', () => {
+        loadTestFiles([
+          {
+            name: _('/test/index.js'),
+            contents: commonJs({
+              inObjectLiteral: ['lib_1/sub_1', 'lib_1/sub_2'],
+            }),
+          },
+          {name: _('/test/package.json'), contents: '{"main": "./index.js"}'},
+          {name: _('/test/index.metadata.json'), contents: 'MOCK METADATA'},
+        ]);
+
+        const {dependencies, missing, deepImports} = createDependencyInfo();
+        host.collectDependencies(_('/test/index.js'), {dependencies, missing, deepImports});
+
+        expect(dependencies.size).toBe(2);
+        expect(missing.size).toBe(0);
+        expect(deepImports.size).toBe(0);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_2'))).toBe(true);
+      });
+
+      it('should recognize imports used for their side-effects only', () => {
+        loadTestFiles([
+          {
+            name: _('/test/index.js'),
+            contents: commonJs({
+              forSideEffects: ['lib_1/sub_1', 'lib_1/sub_2'],
+            }),
+          },
+          {name: _('/test/package.json'), contents: '{"main": "./index.js"}'},
+          {name: _('/test/index.metadata.json'), contents: 'MOCK METADATA'},
+        ]);
+
+        const {dependencies, missing, deepImports} = createDependencyInfo();
+        host.collectDependencies(_('/test/index.js'), {dependencies, missing, deepImports});
+
+        expect(dependencies.size).toBe(2);
+        expect(missing.size).toBe(0);
+        expect(deepImports.size).toBe(0);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_2'))).toBe(true);
+      });
+
+      it('should recognize star re-exports (with both emitted and imported helpers)', () => {
+        loadTestFiles([
+          {
+            name: _('/test/index.js'),
+            contents: commonJs({
+              reExportsWithEmittedHelper: ['lib_1', 'lib_1/sub_1'],
+              reExportsWithImportedHelper: ['lib_1', 'lib_1/sub_2'],
+            }),
+          },
+          {name: _('/test/package.json'), contents: '{"main": "./index.js"}'},
+          {name: _('/test/index.metadata.json'), contents: 'MOCK METADATA'},
+        ]);
+
+        const {dependencies, missing, deepImports} = createDependencyInfo();
+        host.collectDependencies(_('/test/index.js'), {dependencies, missing, deepImports});
+
+        expect(dependencies.size).toBe(3);
+        expect(missing.size).toBe(0);
+        expect(deepImports.size).toBe(0);
+        expect(dependencies.has(_('/node_modules/lib_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_2'))).toBe(true);
+      });
+
+      it('should not get confused by re-exports with a separate `require()` call', () => {
+        loadTestFiles([
+          {
+            name: _('/test/index.js'),
+            contents: commonJs({
+              reExportsWithoutRequire: ['lib_1', 'lib_1/sub_2'],
+            }),
+          },
+          {name: _('/test/package.json'), contents: '{"main": "./index.js"}'},
+          {name: _('/test/index.metadata.json'), contents: 'MOCK METADATA'},
+        ]);
+
+        const {dependencies, missing, deepImports} = createDependencyInfo();
+        host.collectDependencies(_('/test/index.js'), {dependencies, missing, deepImports});
+
+        expect(dependencies.size).toBe(2);
+        expect(missing.size).toBe(0);
+        expect(deepImports.size).toBe(0);
+        expect(dependencies.has(_('/node_modules/lib_1'))).toBe(true);
+        expect(dependencies.has(_('/node_modules/lib_1/sub_2'))).toBe(true);
+      });
+
       it('should capture missing external imports', () => {
         const {dependencies, missing, deepImports} = createDependencyInfo();
         host.collectDependencies(
@@ -224,16 +360,102 @@ runInEachFileSystem(() => {
     });
   });
 
-  function commonJs(importPaths: string[], exportNames: string[] = []) {
+  interface ImportsPerType {
-    const commonJsRequires =
+    // var foo = require('...');
-        importPaths
+    varDeclaration?: string[];
-            .map(
+
-                p =>
+    // var foo = require('...'), bar = require('...');
-                    `var ${p.replace('@angular/', '').replace(/\.?\.?\//g, '').replace(/@/,'')} = require('${p}');`)
+    varDeclarations?: string[][];
-            .join('\n');
+
+    // exports.foo = require('...');
+    propAssignment?: string[];
+
+    // module.exports = {foo: require('...')};
+    inObjectLiteral?: string[];
+
+    // require('...');
+    forSideEffects?: string[];
+
+    // __export(require('...'));
+    reExportsWithEmittedHelper?: string[];
+
+    // tslib_1.__exportStar(require('...'), exports);
+    reExportsWithImportedHelper?: string[];
+
+    // var foo = require('...');
+    // __export(foo);
+    reExportsWithoutRequire?: string[];
+  }
+
+  function commonJs(importsPerType: ImportsPerType | string[], exportNames: string[] = []): string {
+    if (Array.isArray(importsPerType)) {
+      importsPerType = {varDeclaration: importsPerType};
+    }
+
+    const importStatements = generateImportStatements(importsPerType);
     const exportStatements =
-        exportNames.map(e => `  exports.${e.replace(/.+\./, '')} = ${e};`).join('\n');
+        exportNames.map(e => `exports.${e.replace(/.+\./, '')} = ${e};`).join('\n');
-    return `${commonJsRequires}
+
-${exportStatements}`;
+    return `${importStatements}\n\n${exportStatements}`;
+  }
+
+  function generateImportStatements(importsPerType: ImportsPerType): string {
+    const importStatements: string[] = [];
+
+    const {
+      varDeclaration: importsOfTypeVarDeclaration = [],
+      varDeclarations: importsOfTypeVarDeclarations = [],
+      propAssignment: importsOfTypePropAssignment = [],
+      inObjectLiteral: importsOfTypeInObjectLiteral = [],
+      forSideEffects: importsOfTypeForSideEffects = [],
+      reExportsWithEmittedHelper: importsOfTypeReExportsWithEmittedHelper = [],
+      reExportsWithImportedHelper: importsOfTypeReExportsWithImportedHelper = [],
+      reExportsWithoutRequire: importsOfTypeReExportsWithoutRequire = [],
+    } = importsPerType;
+
+    // var foo = require('...');
+    importsOfTypeVarDeclaration.forEach(
+        p => { importStatements.push(`var ${pathToVarName(p)} = require('${p}');`); });
+
+    // var foo = require('...'), bar = require('...');
+    importsOfTypeVarDeclarations.forEach(pp => {
+      const declarations = pp.map(p => `${pathToVarName(p)} = require('${p}')`);
+      importStatements.push(`var ${declarations.join(', ')};`);
+    });
+
+    // exports.foo = require('...');
+    importsOfTypePropAssignment.forEach(
+        p => { importStatements.push(`exports.${pathToVarName(p)} = require('${p}');`); });
+
+    // module.exports = {foo: require('...')};
+    const propAssignments =
+        importsOfTypeInObjectLiteral.map(p => `\n  ${pathToVarName(p)}: require('${p}')`)
+            .join(', ');
+    importStatements.push(`module.exports = {${propAssignments}\n};`);
+
+    // require('...');
+    importsOfTypeForSideEffects.forEach(p => { importStatements.push(`require('${p}');`); });
+
+    // __export(require('...'));
+    importsOfTypeReExportsWithEmittedHelper.forEach(
+        p => { importStatements.push(`__export(require('${p}'));`); });
+
+    // tslib_1.__exportStar(require('...'), exports);
+    importsOfTypeReExportsWithImportedHelper.forEach(
+        p => { importStatements.push(`tslib_1.__exportStar(require('${p}'), exports);`); });
+
+    // var foo = require('...');
+    // __export(foo);
+    importsOfTypeReExportsWithoutRequire.forEach(p => {
+      const varName = pathToVarName(p);
+      importStatements.push(`var ${varName} = require('${p}');`);
+      importStatements.push(`__export(varName);`);
+    });
+
+    return importStatements.join('\n');
+  }
+
+  function pathToVarName(path: string): string {
+    return path.replace(/^@(angular\/)?/, '').replace(/\.{0,2}\//g, '');
   }
 });