Skip to content

Commit

Permalink
refactor(ngcc): move shared code into DependencyHostBase (#37075)
Browse files Browse the repository at this point in the history 
The various dependency hosts had a lot of duplicated code.
This commit refactors them to move this into the base class.

PR Close #37075
  • Loading branch information
@petebacondarwin @atscott
petebacondarwin authored and atscott committed Jun 4, 2020
1 parent c6872c0 commit 8389025
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 183 deletions.
74 changes: 20 additions & 54 deletions packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
53 changes: 50 additions & 3 deletions packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}
}
74 changes: 11 additions & 63 deletions packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

Expand Down
71 changes: 11 additions & 60 deletions packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
6 changes: 3 additions & 3 deletions packages/compiler-cli/ngcc/test/dependencies/esm_dependency_host_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down

0 comments on commit 8389025

Please sign in to comment.