refactor(ngcc): move shared code into DependencyHostBase (angular#37075)

petebacondarwin · atscott · commit 4d69da57cac9 · 2020-06-04T09:22:39.000-07:00
The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close angular#37075
diff --git a/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts
@@ -9,39 +9,22 @@ import * as ts from 'typescript';
 
 import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
 import {isRequireCall, isWildcardReexportStatement, RequireCall} from '../host/commonjs_umd_utils';
+import {isAssignment, isAssignmentStatement} from '../host/esm2015_host';
 
 import {DependencyHostBase} from './dependency_host';
-import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver';
 
 /**
  * Helper functions for computing dependencies.
  */
 export class CommonJsDependencyHost extends DependencyHostBase {
-  /**
-   * Compute the dependencies of the given file.
-   *
-   * @param file An absolute path to the file whose dependencies we want to get.
-   * @param dependencies A set that will have the absolute paths of resolved entry points added to
-   * it.
-   * @param missing A set that will have the dependencies that could not be found added to it.
-   * @param deepImports A set that will have the import paths that exist but cannot be mapped to
-   * entry-points, i.e. deep-imports.
-   * @param alreadySeen A set that is used to track internal dependencies to prevent getting stuck
-   * in a circular dependency loop.
-   */
-  protected recursivelyCollectDependencies(
-      file: AbsoluteFsPath, dependencies: Set<AbsoluteFsPath>, missing: Set<string>,
-      deepImports: Set<AbsoluteFsPath>, alreadySeen: Set<AbsoluteFsPath>): void {
-    const fromContents = this.fs.readFile(file);
-
-    if (!this.hasRequireCalls(fromContents)) {
-      // Avoid parsing the source file as there are no imports.
-      return;
-    }
+  protected canSkipFile(fileContents: string): boolean {
+    return !hasRequireCalls(fileContents);
+  }
 
+  protected extractImports(file: AbsoluteFsPath, fileContents: string): Set<string> {
     // 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);
+        ts.createSourceFile(file, fileContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS);
     const requireCalls: RequireCall[] = [];
 
     for (const stmt of sf.statements) {
@@ -92,37 +75,20 @@ export class CommonJsDependencyHost extends DependencyHostBase {
       }
     }
 
-    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);
-      }
-    }
+    return new Set(requireCalls.map(call => call.arguments[0].text));
   }
+}
 
-  /**
-   * Check whether a source file needs to be parsed for imports.
-   * This is a performance short-circuit, which saves us from creating
-   * a TypeScript AST unnecessarily.
-   *
-   * @param source The content of the source file to check.
-   *
-   * @returns false if there are definitely no require calls
-   * in this file, true otherwise.
-   */
-  private hasRequireCalls(source: string): boolean {
-    return /require\(['"]/.test(source);
-  }
+/**
+ * Check whether a source file needs to be parsed for imports.
+ * This is a performance short-circuit, which saves us from creating
+ * a TypeScript AST unnecessarily.
+ *
+ * @param source The content of the source file to check.
+ *
+ * @returns false if there are definitely no require calls
+ * in this file, true otherwise.
+ */
+export function hasRequireCalls(source: string): boolean {
+  return /require\(['"]/.test(source);
 }
diff --git a/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts
@@ -9,7 +9,7 @@ import {AbsoluteFsPath, FileSystem, PathSegment} from '../../../src/ngtsc/file_s
 import {EntryPoint} from '../packages/entry_point';
 import {resolveFileWithPostfixes} from '../utils';
 
-import {ModuleResolver} from './module_resolver';
+import {ModuleResolver, ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver';
 
 export interface DependencyHost {
   collectDependencies(
@@ -65,7 +65,54 @@ export abstract class DependencyHostBase implements DependencyHost {
    * @param alreadySeen A set that is used to track internal dependencies to prevent getting stuck
    * in a circular dependency loop.
    */
-  protected abstract recursivelyCollectDependencies(
+  protected recursivelyCollectDependencies(
       file: AbsoluteFsPath, dependencies: Set<AbsoluteFsPath>, missing: Set<string>,
-      deepImports: Set<AbsoluteFsPath>, alreadySeen: Set<AbsoluteFsPath>): void;
+      deepImports: Set<string>, alreadySeen: Set<AbsoluteFsPath>): void {
+    const fromContents = this.fs.readFile(file);
+    if (this.canSkipFile(fromContents)) {
+      return;
+    }
+    const imports = this.extractImports(file, fromContents);
+    for (const importPath of imports) {
+      const resolved =
+          this.processImport(importPath, file, dependencies, missing, deepImports, alreadySeen);
+      if (!resolved) {
+        missing.add(importPath);
+      }
+    }
+  }
+
+  protected abstract canSkipFile(fileContents: string): boolean;
+  protected abstract extractImports(file: AbsoluteFsPath, fileContents: string): Set<string>;
+
+  /**
+   * Resolve the given `importPath` from `file` and add it to the appropriate set.
+   *
+   * If the import is local to this package then follow it by calling
+   * `recursivelyCollectDependencies()`.
+   *
+   * @returns `true` if the import was resolved (to an entry-point, a local import, or a
+   * deep-import), `false` otherwise.
+   */
+  protected processImport(
+      importPath: string, file: AbsoluteFsPath, dependencies: Set<AbsoluteFsPath>,
+      missing: Set<string>, deepImports: Set<string>, alreadySeen: Set<AbsoluteFsPath>): boolean {
+    const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file);
+    if (resolvedModule === null) {
+      return false;
+    }
+    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);
+    }
+    return true;
+  }
 }
diff --git a/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts
@@ -8,77 +8,25 @@
 import * as ts from 'typescript';
 import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
 import {DependencyHostBase} from './dependency_host';
-import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver';
 
 /**
  * Helper functions for computing dependencies.
  */
 export class EsmDependencyHost extends DependencyHostBase {
-  /**
-   * Compute the dependencies of the given file.
-   *
-   * @param file An absolute path to the file whose dependencies we want to get.
-   * @param dependencies A set that will have the absolute paths of resolved entry points added to
-   * it.
-   * @param missing A set that will have the dependencies that could not be found added to it.
-   * @param deepImports A set that will have the import paths that exist but cannot be mapped to
-   * entry-points, i.e. deep-imports.
-   * @param alreadySeen A set that is used to track internal dependencies to prevent getting stuck
-   * in a circular dependency loop.
-   */
-  protected recursivelyCollectDependencies(
-      file: AbsoluteFsPath, dependencies: Set<AbsoluteFsPath>, missing: Set<string>,
-      deepImports: Set<string>, alreadySeen: Set<AbsoluteFsPath>): void {
-    const fromContents = this.fs.readFile(file);
-
-    if (!hasImportOrReexportStatements(fromContents)) {
-      // Avoid parsing the source file as there are no imports.
-      return;
-    }
+  protected canSkipFile(fileContents: string): boolean {
+    return !hasImportOrReexportStatements(fileContents);
+  }
 
+  protected extractImports(file: AbsoluteFsPath, fileContents: string): Set<string> {
+    const imports: string[] = [];
     // 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);
-    sf.statements
-        // filter out statements that are not imports or reexports
-        .filter(isStringImportOrReexport)
-        // Grab the id of the module that is being imported
-        .map(stmt => stmt.moduleSpecifier.text)
-        .forEach(importPath => {
-          const resolved =
-              this.processImport(importPath, file, dependencies, missing, deepImports, alreadySeen);
-          if (!resolved) {
-            missing.add(importPath);
-          }
-        });
-  }
-
-  /**
-   * Resolve the given `importPath` from `file` and add it to the appropriate set.
-   *
-   * @returns `true` if the import was resolved (to an entry-point, a local import, or a
-   * deep-import).
-   */
-  protected processImport(
-      importPath: string, file: AbsoluteFsPath, dependencies: Set<AbsoluteFsPath>,
-      missing: Set<string>, deepImports: Set<string>, alreadySeen: Set<AbsoluteFsPath>): boolean {
-    const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file);
-    if (resolvedModule === null) {
-      return false;
-    }
-    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);
-    }
-    return true;
+        ts.createSourceFile(file, fileContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS);
+    return new Set(sf.statements
+                       // filter out statements that are not imports or reexports
+                       .filter(isStringImportOrReexport)
+                       // Grab the id of the module that is being imported
+                       .map(stmt => stmt.moduleSpecifier.text));
   }
 }
 
diff --git a/packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts
@@ -6,85 +6,36 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import * as ts from 'typescript';
+
 import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
 import {getImportsOfUmdModule, parseStatementForUmdModule} from '../host/umd_host';
+
+import {hasRequireCalls} from './commonjs_dependency_host';
 import {DependencyHostBase} from './dependency_host';
-import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver';
 
 /**
  * Helper functions for computing dependencies.
  */
 export class UmdDependencyHost extends DependencyHostBase {
-  /**
-   * Compute the dependencies of the given file.
-   *
-   * @param file An absolute path to the file whose dependencies we want to get.
-   * @param dependencies A set that will have the absolute paths of resolved entry points added to
-   * it.
-   * @param missing A set that will have the dependencies that could not be found added to it.
-   * @param deepImports A set that will have the import paths that exist but cannot be mapped to
-   * entry-points, i.e. deep-imports.
-   * @param alreadySeen A set that is used to track internal dependencies to prevent getting stuck
-   * in a circular dependency loop.
-   */
-  protected recursivelyCollectDependencies(
-      file: AbsoluteFsPath, dependencies: Set<AbsoluteFsPath>, missing: Set<string>,
-      deepImports: Set<string>, alreadySeen: Set<AbsoluteFsPath>): void {
-    const fromContents = this.fs.readFile(file);
-
-    if (!this.hasRequireCalls(fromContents)) {
-      // Avoid parsing the source file as there are no imports.
-      return;
-    }
+  protected canSkipFile(fileContents: string): boolean {
+    return !hasRequireCalls(fileContents);
+  }
 
+  protected extractImports(file: AbsoluteFsPath, fileContents: string): Set<string> {
     // 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);
+        ts.createSourceFile(file, fileContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS);
 
     if (sf.statements.length !== 1) {
-      return;
+      return new Set();
     }
 
     const umdModule = parseStatementForUmdModule(sf.statements[0]);
     const umdImports = umdModule && getImportsOfUmdModule(umdModule);
     if (umdImports === null) {
-      return;
+      return new Set();
     }
 
-    umdImports.forEach(umdImport => {
-      const resolvedModule = this.moduleResolver.resolveModuleImport(umdImport.path, file);
-      if (resolvedModule) {
-        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(umdImport.path);
-      }
-    });
-  }
-
-  /**
-   * Check whether a source file needs to be parsed for imports.
-   * This is a performance short-circuit, which saves us from creating
-   * a TypeScript AST unnecessarily.
-   *
-   * @param source The content of the source file to check.
-   *
-   * @returns false if there are definitely no require calls
-   * in this file, true otherwise.
-   */
-  private hasRequireCalls(source: string): boolean {
-    return /require\(['"]/.test(source);
+    return new Set(umdImports.map(i => i.path));
   }
 }
diff --git a/packages/compiler-cli/ngcc/test/dependencies/esm_dependency_host_spec.ts b/packages/compiler-cli/ngcc/test/dependencies/esm_dependency_host_spec.ts
@@ -27,12 +27,12 @@ runInEachFileSystem(() => {
     });
 
     describe('collectDependencies()', () => {
-      it('should not generate a TS AST if the source does not contain any imports or re-exports',
+      it('should not try to extract import paths if the source does not contain any imports or re-exports',
          () => {
-           spyOn(ts, 'createSourceFile');
+           const extractImportsSpy = spyOn(host as any, 'extractImports');
            host.collectDependencies(
                _('/no/imports/or/re-exports/index.js'), createDependencyInfo());
-           expect(ts.createSourceFile).not.toHaveBeenCalled();
+           expect(extractImportsSpy).not.toHaveBeenCalled();
          });
 
       it('should resolve all the external imports of the source file', () => {
