From ae142a682783043890b3d795da0791d7070bc227 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 18 Aug 2019 21:04:11 +0200 Subject: [PATCH] refactor(ngcc): avoid repeated file resolution during dependency scan (#32181) During the recursive processing of dependencies, ngcc resolves the requested file to an actual location on disk, by testing various extensions. For recursive calls however, the path is known to have been resolved in the module resolver. Therefore, it is safe to move the path resolution to the initial caller into the recursive process. Note that this is not expected to improve the performance of ngcc, as the call to `resolveFileWithPostfixes` is known to succeed immediately, as the provided path is known to exist without needing to add any postfixes. Furthermore, the FileSystem caches whether files exist, so the additional check that we used to do was cheap. PR Close #32181 --- .../src/dependencies/commonjs_dependency_host.ts | 13 ++++--------- .../ngcc/src/dependencies/dependency_host.ts | 13 ++++++++++--- .../ngcc/src/dependencies/esm_dependency_host.ts | 13 ++++--------- .../ngcc/src/dependencies/umd_dependency_host.ts | 13 ++++--------- 4 files changed, 22 insertions(+), 30 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts index 2fc8338b50..01e9d55e9c 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts @@ -8,7 +8,6 @@ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; import {isRequireCall} from '../host/commonjs_host'; -import {resolveFileWithPostfixes} from '../utils'; import {DependencyHostBase} from './dependency_host'; import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver'; @@ -31,11 +30,7 @@ export class CommonJsDependencyHost extends DependencyHostBase { protected recursivelyFindDependencies( file: AbsoluteFsPath, dependencies: Set, missing: Set, deepImports: Set, alreadySeen: Set): void { - const resolvedFile = resolveFileWithPostfixes(this.fs, file, ['', '.js', '/index.js']); - if (resolvedFile === null) { - return; - } - const fromContents = this.fs.readFile(resolvedFile); + const fromContents = this.fs.readFile(file); if (!this.hasRequireCalls(fromContents)) { // Avoid parsing the source file as there are no imports. @@ -43,8 +38,8 @@ 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( - resolvedFile, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS); + const sf = + ts.createSourceFile(file, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS); for (const statement of sf.statements) { const declarations = @@ -52,7 +47,7 @@ export class CommonJsDependencyHost extends DependencyHostBase { for (const declaration of declarations) { if (declaration.initializer && isRequireCall(declaration.initializer)) { const importPath = declaration.initializer.arguments[0].text; - const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, resolvedFile); + const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file); if (resolvedModule) { if (resolvedModule instanceof ResolvedRelativeModule) { const internalDependency = resolvedModule.modulePath; diff --git a/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts index b5b2a618d5..900b44a992 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ import {AbsoluteFsPath, FileSystem, PathSegment} from '../../../src/ngtsc/file_system'; +import {resolveFileWithPostfixes} from '../utils'; + import {ModuleResolver} from './module_resolver'; export interface DependencyHost { @@ -32,10 +34,15 @@ export abstract class DependencyHostBase implements DependencyHost { const dependencies = new Set(); const missing = new Set(); const deepImports = new Set(); - const alreadySeen = new Set(); - this.recursivelyFindDependencies( - entryPointPath, dependencies, missing, deepImports, alreadySeen); + const resolvedFile = + resolveFileWithPostfixes(this.fs, entryPointPath, ['', '.js', '/index.js']); + if (resolvedFile !== null) { + const alreadySeen = new Set(); + this.recursivelyFindDependencies( + resolvedFile, dependencies, missing, deepImports, alreadySeen); + } + return {dependencies, missing, deepImports}; } diff --git a/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts index e164328282..2b6b394d85 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts @@ -7,7 +7,6 @@ */ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; -import {resolveFileWithPostfixes} from '../utils'; import {DependencyHostBase} from './dependency_host'; import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver'; @@ -30,11 +29,7 @@ export class EsmDependencyHost extends DependencyHostBase { protected recursivelyFindDependencies( file: AbsoluteFsPath, dependencies: Set, missing: Set, deepImports: Set, alreadySeen: Set): void { - const resolvedFile = resolveFileWithPostfixes(this.fs, file, ['', '.js', '/index.js']); - if (resolvedFile === null) { - return; - } - const fromContents = this.fs.readFile(resolvedFile); + const fromContents = this.fs.readFile(file); if (!hasImportOrReexportStatements(fromContents)) { // Avoid parsing the source file as there are no imports. @@ -42,8 +37,8 @@ export class EsmDependencyHost extends DependencyHostBase { } // Parse the source into a TypeScript AST and then walk it looking for imports and re-exports. - const sf = ts.createSourceFile( - resolvedFile, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS); + 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) @@ -51,7 +46,7 @@ export class EsmDependencyHost extends DependencyHostBase { .map(stmt => stmt.moduleSpecifier.text) // Resolve this module id into an absolute path .forEach(importPath => { - const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, resolvedFile); + const resolvedModule = this.moduleResolver.resolveModuleImport(importPath, file); if (resolvedModule) { if (resolvedModule instanceof ResolvedRelativeModule) { const internalDependency = resolvedModule.modulePath; diff --git a/packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts index 64ca73d25a..0664dfce88 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/umd_dependency_host.ts @@ -8,7 +8,6 @@ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; import {getImportsOfUmdModule, parseStatementForUmdModule} from '../host/umd_host'; -import {resolveFileWithPostfixes} from '../utils'; import {DependencyHostBase} from './dependency_host'; import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver'; @@ -31,11 +30,7 @@ export class UmdDependencyHost extends DependencyHostBase { protected recursivelyFindDependencies( file: AbsoluteFsPath, dependencies: Set, missing: Set, deepImports: Set, alreadySeen: Set): void { - const resolvedFile = resolveFileWithPostfixes(this.fs, file, ['', '.js', '/index.js']); - if (resolvedFile === null) { - return; - } - const fromContents = this.fs.readFile(resolvedFile); + const fromContents = this.fs.readFile(file); if (!this.hasRequireCalls(fromContents)) { // Avoid parsing the source file as there are no imports. @@ -43,8 +38,8 @@ export class UmdDependencyHost extends DependencyHostBase { } // Parse the source into a TypeScript AST and then walk it looking for imports and re-exports. - const sf = ts.createSourceFile( - resolvedFile, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS); + const sf = + ts.createSourceFile(file, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS); if (sf.statements.length !== 1) { return; @@ -57,7 +52,7 @@ export class UmdDependencyHost extends DependencyHostBase { } umdImports.forEach(umdImport => { - const resolvedModule = this.moduleResolver.resolveModuleImport(umdImport.path, resolvedFile); + const resolvedModule = this.moduleResolver.resolveModuleImport(umdImport.path, file); if (resolvedModule) { if (resolvedModule instanceof ResolvedRelativeModule) { const internalDependency = resolvedModule.modulePath;