From 17d5e2bc9921375c17811987d963faa4b3bc283a Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 20 Dec 2019 15:38:11 +0200 Subject: [PATCH] refactor(ngcc): share code between `CommonJsReflectionHost` and `UmdReflectionHost` (#34512) While different, CommonJS and UMD have a lot in common regarding the their exports are constructed. Therefore, there was some code duplication between `CommonJsReflectionHost` and `UmdReflectionHost`. This commit extracts some of the common bits into a separate file as helpers to allow reusing the code in both `ReflectionHost`s. PR Close #34512 --- .../dependencies/commonjs_dependency_host.ts | 2 +- .../ngcc/src/host/commonjs_host.ts | 74 ++-------- .../ngcc/src/host/commonjs_umd_utils.ts | 85 ++++++++++++ .../compiler-cli/ngcc/src/host/umd_host.ts | 128 ++++++------------ .../rendering/commonjs_rendering_formatter.ts | 2 +- packages/compiler-cli/ngcc/src/utils.ts | 7 + packages/compiler-cli/ngcc/test/utils_spec.ts | 40 +++++- 7 files changed, 179 insertions(+), 159 deletions(-) create mode 100644 packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts 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 5bab3a7bc4..9825e3a262 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts +++ b/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_host'; +import {isRequireCall} from '../host/commonjs_umd_utils'; import {DependencyHostBase} from './dependency_host'; import {ResolvedDeepImport, ResolvedRelativeModule} from './module_resolver'; diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts index 817dac68d9..1818ede3d8 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts @@ -11,8 +11,9 @@ import {absoluteFrom} from '../../../src/ngtsc/file_system'; import {Declaration, Import} from '../../../src/ngtsc/reflection'; import {Logger} from '../logging/logger'; import {BundleProgram} from '../packages/bundle_program'; -import {isDefined, stripExtension} from '../utils'; +import {getOrDefault, isDefined, stripExtension} from '../utils'; +import {ExportDeclaration, ExportStatement, ReexportStatement, RequireCall, findNamespaceOfIdentifier, findRequireCallReference, isExportStatement, isReexportStatement, isRequireCall} from './commonjs_umd_utils'; import {Esm5ReflectionHost} from './esm5_host'; import {NgccClassSymbol} from './ngcc_host'; @@ -103,7 +104,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { private computeExportsOfCommonJsModule(sourceFile: ts.SourceFile): Map { const moduleMap = new Map(); for (const statement of this.getModuleStatements(sourceFile)) { - if (isCommonJsExportStatement(statement)) { + if (isExportStatement(statement)) { const exportDeclaration = this.extractCommonJsExportDeclaration(statement); moduleMap.set(exportDeclaration.name, exportDeclaration.declaration); } else if (isReexportStatement(statement)) { @@ -116,8 +117,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { return moduleMap; } - private extractCommonJsExportDeclaration(statement: CommonJsExportStatement): - CommonJsExportDeclaration { + private extractCommonJsExportDeclaration(statement: ExportStatement): ExportDeclaration { const exportExpression = statement.expression.right; const declaration = this.getDeclarationOfExpression(exportExpression); const name = statement.expression.left.name.text; @@ -136,12 +136,12 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { } private extractCommonJsReexports(statement: ReexportStatement, containingFile: ts.SourceFile): - CommonJsExportDeclaration[] { + ExportDeclaration[] { const reexportArg = statement.expression.arguments[0]; const requireCall = isRequireCall(reexportArg) ? reexportArg : - ts.isIdentifier(reexportArg) ? this.findRequireCallReference(reexportArg) : null; + ts.isIdentifier(reexportArg) ? findRequireCallReference(reexportArg, this.checker) : null; if (requireCall === null) { return []; } @@ -158,7 +158,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { } const viaModule = stripExtension(importedFile.fileName); - const reexports: CommonJsExportDeclaration[] = []; + const reexports: ExportDeclaration[] = []; importedExports.forEach((decl, name) => { if (decl.node !== null) { reexports.push({name, declaration: {node: decl.node, viaModule}}); @@ -173,15 +173,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { // Is `id` a namespaced property access, e.g. `Directive` in `core.Directive`? // If so capture the symbol of the namespace, e.g. `core`. const nsIdentifier = findNamespaceOfIdentifier(id); - return nsIdentifier && this.findRequireCallReference(nsIdentifier); - } - - private findRequireCallReference(id: ts.Identifier): RequireCall|null { - const symbol = id && this.checker.getSymbolAtLocation(id) || null; - const declaration = symbol && symbol.valueDeclaration; - const initializer = - declaration && ts.isVariableDeclaration(declaration) && declaration.initializer || null; - return initializer && isRequireCall(initializer) ? initializer : null; + return nsIdentifier && findRequireCallReference(nsIdentifier, this.checker); } private getCommonJsImportedDeclaration(id: ts.Identifier): Declaration|null { @@ -215,53 +207,3 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { } } } - -type CommonJsExportStatement = ts.ExpressionStatement & { - expression: - ts.BinaryExpression & {left: ts.PropertyAccessExpression & {expression: ts.Identifier}} -}; -export function isCommonJsExportStatement(s: ts.Statement): s is CommonJsExportStatement { - return ts.isExpressionStatement(s) && ts.isBinaryExpression(s.expression) && - ts.isPropertyAccessExpression(s.expression.left) && - ts.isIdentifier(s.expression.left.expression) && - s.expression.left.expression.text === 'exports'; -} - -interface CommonJsExportDeclaration { - name: string; - declaration: Declaration; -} - -export type RequireCall = ts.CallExpression & {arguments: [ts.StringLiteral]}; -export function isRequireCall(node: ts.Node): node is RequireCall { - return ts.isCallExpression(node) && ts.isIdentifier(node.expression) && - node.expression.text === 'require' && node.arguments.length === 1 && - ts.isStringLiteral(node.arguments[0]); -} - -/** - * If the identifier `id` is the RHS of a property access of the form `namespace.id` - * and `namespace` is an identifer then return `namespace`, otherwise `null`. - * @param id The identifier whose namespace we want to find. - */ -function findNamespaceOfIdentifier(id: ts.Identifier): ts.Identifier|null { - return id.parent && ts.isPropertyAccessExpression(id.parent) && - ts.isIdentifier(id.parent.expression) ? - id.parent.expression : - null; -} - -type ReexportStatement = ts.ExpressionStatement & {expression: ts.CallExpression}; -function isReexportStatement(statement: ts.Statement): statement is ReexportStatement { - return ts.isExpressionStatement(statement) && ts.isCallExpression(statement.expression) && - ts.isIdentifier(statement.expression.expression) && - statement.expression.expression.text === '__export' && - statement.expression.arguments.length === 1; -} - -function getOrDefault(map: Map, key: K, factory: (key: K) => V): V { - if (!map.has(key)) { - map.set(key, factory(key)); - } - return map.get(key) !; -} diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts b/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts new file mode 100644 index 0000000000..27bc1597b8 --- /dev/null +++ b/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts @@ -0,0 +1,85 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as ts from 'typescript'; +import {Declaration} from '../../../src/ngtsc/reflection'; + + +export interface ExportDeclaration { + name: string; + declaration: Declaration; +} + +export interface ExportStatement extends ts.ExpressionStatement { + expression: ts.BinaryExpression&{left: ts.PropertyAccessExpression & {expression: ts.Identifier}}; +} + +export interface ReexportStatement extends ts.ExpressionStatement { expression: ts.CallExpression; } + +export interface RequireCall extends ts.CallExpression { + arguments: ts.CallExpression['arguments']&[ts.StringLiteral]; +} + + +/** + * Return the "namespace" of the specified `ts.Identifier` if the identifier is the RHS of a + * property access expression, i.e. an expression of the form `.` (in which case a + * `ts.Identifier` corresponding to `` will be returned). Otherwise return `null`. + */ +export function findNamespaceOfIdentifier(id: ts.Identifier): ts.Identifier|null { + return id.parent && ts.isPropertyAccessExpression(id.parent) && + ts.isIdentifier(id.parent.expression) ? + id.parent.expression : + null; +} + +/** + * Return the `RequireCall` that is used to initialize the specified `ts.Identifier`, if the + * specified indentifier was indeed initialized with a require call in a declaration of the form: + * `var = require('...')` + */ +export function findRequireCallReference(id: ts.Identifier, checker: ts.TypeChecker): RequireCall| + null { + const symbol = checker.getSymbolAtLocation(id) || null; + const declaration = symbol && symbol.valueDeclaration; + const initializer = + declaration && ts.isVariableDeclaration(declaration) && declaration.initializer || null; + return initializer && isRequireCall(initializer) ? initializer : null; +} + +/** + * Check whether the specified `ts.Statement` is an export statement, i.e. an expression statement + * of the form: `export. = ` + */ +export function isExportStatement(stmt: ts.Statement): stmt is ExportStatement { + return ts.isExpressionStatement(stmt) && ts.isBinaryExpression(stmt.expression) && + (stmt.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken) && + ts.isPropertyAccessExpression(stmt.expression.left) && + ts.isIdentifier(stmt.expression.left.expression) && + stmt.expression.left.expression.text === 'exports'; +} + +/** + * Check whether the specified `ts.Statement` is a re-export statement, i.e. an expression statement + * of the form: `__export()` + */ +export function isReexportStatement(stmt: ts.Statement): stmt is ReexportStatement { + return ts.isExpressionStatement(stmt) && ts.isCallExpression(stmt.expression) && + ts.isIdentifier(stmt.expression.expression) && + stmt.expression.expression.text === '__export' && stmt.expression.arguments.length === 1; +} + +/** + * Check whether the specified `ts.Node` represents a `require()` call, i.e. an call expression of + * the form: `require('')` + */ +export function isRequireCall(node: ts.Node): node is RequireCall { + return ts.isCallExpression(node) && ts.isIdentifier(node.expression) && + node.expression.text === 'require' && node.arguments.length === 1 && + ts.isStringLiteral(node.arguments[0]); +} diff --git a/packages/compiler-cli/ngcc/src/host/umd_host.ts b/packages/compiler-cli/ngcc/src/host/umd_host.ts index 85368cdab3..b92aef5bad 100644 --- a/packages/compiler-cli/ngcc/src/host/umd_host.ts +++ b/packages/compiler-cli/ngcc/src/host/umd_host.ts @@ -12,7 +12,8 @@ import {absoluteFrom} from '../../../src/ngtsc/file_system'; import {Declaration, Import} from '../../../src/ngtsc/reflection'; import {Logger} from '../logging/logger'; import {BundleProgram} from '../packages/bundle_program'; -import {stripExtension} from '../utils'; +import {getOrDefault, stripExtension} from '../utils'; +import {ExportDeclaration, ExportStatement, ReexportStatement, findNamespaceOfIdentifier, findRequireCallReference, isExportStatement, isReexportStatement, isRequireCall} from './commonjs_umd_utils'; import {Esm5ReflectionHost, stripParentheses} from './esm5_host'; export class UmdReflectionHost extends Esm5ReflectionHost { @@ -54,47 +55,45 @@ export class UmdReflectionHost extends Esm5ReflectionHost { if (sourceFile.isDeclarationFile) { return null; } - if (!this.umdModules.has(sourceFile)) { - if (sourceFile.statements.length !== 1) { + + return getOrDefault(this.umdModules, sourceFile, (sf: ts.SourceFile) => { + if (sf.statements.length !== 1) { throw new Error( - `Expected UMD module file (${sourceFile.fileName}) to contain exactly one statement, but found ${sourceFile.statements}.`); + `Expected UMD module file (${sf.fileName}) to contain exactly one statement, but found ${sf.statements.length}.`); } - this.umdModules.set(sourceFile, parseStatementForUmdModule(sourceFile.statements[0])); - } - return this.umdModules.get(sourceFile) !; + return parseStatementForUmdModule(sf.statements[0]); + }); } getUmdImportPath(importParameter: ts.ParameterDeclaration): string|null { - if (this.umdImportPaths.has(importParameter)) { - return this.umdImportPaths.get(importParameter) !; - } - - const umdModule = this.getUmdModule(importParameter.getSourceFile()); - if (umdModule === null) { - return null; - } - - const imports = getImportsOfUmdModule(umdModule); - if (imports === null) { - return null; - } - - for (const i of imports) { - this.umdImportPaths.set(i.parameter, i.path); - if (i.parameter === importParameter) { - return i.path; + return getOrDefault(this.umdImportPaths, importParameter, (param: ts.ParameterDeclaration) => { + const umdModule = this.getUmdModule(param.getSourceFile()); + if (umdModule === null) { + return null; } - } - return null; + const imports = getImportsOfUmdModule(umdModule); + if (imports === null) { + return null; + } + + let importPath: string|null = null; + + for (const i of imports) { + // Add all imports to the map to speed up future look ups. + this.umdImportPaths.set(i.parameter, i.path); + if (i.parameter === importParameter) { + importPath = i.path; + } + } + + return importPath; + }); } getUmdExports(sourceFile: ts.SourceFile): Map|null { - if (!this.umdExports.has(sourceFile)) { - const moduleExports = this.computeExportsOfUmdModule(sourceFile); - this.umdExports.set(sourceFile, moduleExports); - } - return this.umdExports.get(sourceFile) !; + return getOrDefault( + this.umdExports, sourceFile, (sf: ts.SourceFile) => this.computeExportsOfUmdModule(sf)); } /** Get the top level statements for a module. @@ -112,9 +111,9 @@ export class UmdReflectionHost extends Esm5ReflectionHost { private computeExportsOfUmdModule(sourceFile: ts.SourceFile): Map|null { const moduleMap = new Map(); for (const statement of this.getModuleStatements(sourceFile)) { - if (isUmdExportStatement(statement)) { - const declaration = this.extractUmdExportDeclaration(statement); - moduleMap.set(declaration.name, declaration.declaration); + if (isExportStatement(statement)) { + const exportDeclaration = this.extractUmdExportDeclaration(statement); + moduleMap.set(exportDeclaration.name, exportDeclaration.declaration); } else if (isReexportStatement(statement)) { const reexports = this.extractUmdReexports(statement, sourceFile); for (const reexport of reexports) { @@ -125,7 +124,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost { return moduleMap; } - private extractUmdExportDeclaration(statement: UmdExportStatement): UmdExportDeclaration { + private extractUmdExportDeclaration(statement: ExportStatement): ExportDeclaration { const exportExpression = statement.expression.right; const declaration = this.getDeclarationOfExpression(exportExpression); const name = statement.expression.left.name.text; @@ -144,12 +143,12 @@ export class UmdReflectionHost extends Esm5ReflectionHost { } private extractUmdReexports(statement: ReexportStatement, containingFile: ts.SourceFile): - UmdExportDeclaration[] { + ExportDeclaration[] { const reexportArg = statement.expression.arguments[0]; const requireCall = isRequireCall(reexportArg) ? reexportArg : - ts.isIdentifier(reexportArg) ? this.findRequireCallReference(reexportArg) : null; + ts.isIdentifier(reexportArg) ? findRequireCallReference(reexportArg, this.checker) : null; let importPath: string|null = null; @@ -175,7 +174,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost { } const viaModule = stripExtension(importedFile.fileName); - const reexports: UmdExportDeclaration[] = []; + const reexports: ExportDeclaration[] = []; importedExports.forEach((decl, name) => { if (decl.node !== null) { reexports.push({name, declaration: {node: decl.node, viaModule}}); @@ -196,14 +195,6 @@ export class UmdReflectionHost extends Esm5ReflectionHost { return declaration && ts.isParameter(declaration) ? declaration : null; } - private findRequireCallReference(id: ts.Identifier): RequireCall|null { - const symbol = id && this.checker.getSymbolAtLocation(id) || null; - const declaration = symbol && symbol.valueDeclaration; - const initializer = - declaration && ts.isVariableDeclaration(declaration) && declaration.initializer || null; - return initializer && isRequireCall(initializer) ? initializer : null; - } - private getUmdImportedDeclaration(id: ts.Identifier): Declaration|null { const importInfo = this.getImportOfIdentifier(id); if (importInfo === null) { @@ -282,31 +273,6 @@ interface UmdModule { factoryFn: ts.FunctionExpression; } -type UmdExportStatement = ts.ExpressionStatement & { - expression: - ts.BinaryExpression & {left: ts.PropertyAccessExpression & {expression: ts.Identifier}} -}; - -function isUmdExportStatement(s: ts.Statement): s is UmdExportStatement { - return ts.isExpressionStatement(s) && ts.isBinaryExpression(s.expression) && - ts.isPropertyAccessExpression(s.expression.left) && - ts.isIdentifier(s.expression.left.expression) && - s.expression.left.expression.text === 'exports'; -} - -interface UmdExportDeclaration { - name: string; - declaration: Declaration; -} - -type ReexportStatement = ts.ExpressionStatement & {expression: ts.CallExpression}; -function isReexportStatement(statement: ts.Statement): statement is ReexportStatement { - return ts.isExpressionStatement(statement) && ts.isCallExpression(statement.expression) && - ts.isIdentifier(statement.expression.expression) && - statement.expression.expression.text === '__export' && - statement.expression.arguments.length === 1; -} - function getRequiredModulePath(wrapperFn: ts.FunctionExpression, paramIndex: number): string { const statement = wrapperFn.body.statements[0]; if (!ts.isExpressionStatement(statement)) { @@ -335,21 +301,3 @@ function getRequiredModulePath(wrapperFn: ts.FunctionExpression, paramIndex: num } } } - -type RequireCall = ts.CallExpression & {arguments: [ts.StringLiteral]}; -function isRequireCall(node: ts.Node): node is RequireCall { - return ts.isCallExpression(node) && ts.isIdentifier(node.expression) && - node.expression.text === 'require' && node.arguments.length === 1; -} - -/** - * If the identifier `id` is the RHS of a property access of the form `namespace.id` - * and `namespace` is an identifer then return `namespace`, otherwise `null`. - * @param id The identifier whose namespace we want to find. - */ -function findNamespaceOfIdentifier(id: ts.Identifier): ts.Identifier|null { - return id.parent && ts.isPropertyAccessExpression(id.parent) && - ts.isIdentifier(id.parent.expression) ? - id.parent.expression : - null; -} diff --git a/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts index 30938fc5ac..bdac597093 100644 --- a/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts @@ -11,7 +11,7 @@ import MagicString from 'magic-string'; import {Reexport} from '../../../src/ngtsc/imports'; import {Import, ImportManager} from '../../../src/ngtsc/translator'; import {ExportInfo} from '../analysis/private_declarations_analyzer'; -import {isRequireCall} from '../host/commonjs_host'; +import {isRequireCall} from '../host/commonjs_umd_utils'; import {NgccReflectionHost} from '../host/ngcc_host'; import {Esm5RenderingFormatter} from './esm5_rendering_formatter'; import {stripExtension} from './utils'; diff --git a/packages/compiler-cli/ngcc/src/utils.ts b/packages/compiler-cli/ngcc/src/utils.ts index 929dffa02b..9946b32ca2 100644 --- a/packages/compiler-cli/ngcc/src/utils.ts +++ b/packages/compiler-cli/ngcc/src/utils.ts @@ -66,6 +66,13 @@ export function findAll(node: ts.Node, test: (node: ts.Node) => node is ts.No } } +export function getOrDefault(map: Map, key: K, factory: (key: K) => V): V { + if (!map.has(key)) { + map.set(key, factory(key)); + } + return map.get(key) !; +} + /** * Does the given declaration have a name which is an identifier? * @param declaration The declaration to test. diff --git a/packages/compiler-cli/ngcc/test/utils_spec.ts b/packages/compiler-cli/ngcc/test/utils_spec.ts index 4b44d7e6d2..fafe355e12 100644 --- a/packages/compiler-cli/ngcc/test/utils_spec.ts +++ b/packages/compiler-cli/ngcc/test/utils_spec.ts @@ -6,7 +6,31 @@ * found in the LICENSE file at https://angular.io/license */ -import {isRelativePath} from '../src/utils'; +import {getOrDefault, isRelativePath, stripExtension} from '../src/utils'; + +describe('getOrDefault()', () => { + it('should return an existing value', () => { + const map = new Map([['k1', 'v1'], ['k2', 'v2']]); + const factorySpy = jasmine.createSpy('factory'); + + expect(getOrDefault(map, 'k1', factorySpy)).toBe('v1'); + expect(getOrDefault(map, 'k2', factorySpy)).toBe('v2'); + expect(factorySpy).not.toHaveBeenCalled(); + }); + + it('should create, store and return the value if it does not exist', () => { + const map = new Map([['k1', 'v1'], ['k2', 'v2']]); + const factorySpy = jasmine.createSpy('factory').and.returnValues('v3', 'never gonna happen'); + + expect(getOrDefault(map, 'k3', factorySpy)).toBe('v3'); + expect(factorySpy).toHaveBeenCalledTimes(1); + + factorySpy.calls.reset(); + + expect(getOrDefault(map, 'k3', factorySpy)).toBe('v3'); + expect(factorySpy).not.toHaveBeenCalled(); + }); +}); describe('isRelativePath()', () => { it('should return true for relative paths', () => { @@ -34,3 +58,17 @@ describe('isRelativePath()', () => { expect(isRelativePath('@abc/xyz')).toBe(false); }); }); + +describe('stripExtension()', () => { + it('should strip the extension from a file name', () => { + expect(stripExtension('foo.ts')).toBe('foo'); + expect(stripExtension('/foo/bar.ts')).toBe('/foo/bar'); + expect(stripExtension('/foo/bar.d.ts')).toBe('/foo/bar'); + }); + + it('should do nothing if there is no extension in a file name', () => { + expect(stripExtension('foo')).toBe('foo'); + expect(stripExtension('/foo/bar')).toBe('/foo/bar'); + expect(stripExtension('/fo-o/b_ar')).toBe('/fo-o/b_ar'); + }); +});