From 91092f668e24d6c469f2d39559f4350176406b9c Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 12 May 2020 08:20:01 +0100 Subject: [PATCH] fix(ngcc): support `defineProperty()` re-exports in CommonJS and UMD (#36989) In TypeScript 3.9 some re-export syntaxes have changed to be getter functions (created by calls to `Object.defineProperty()`) rather than simple property accessors. This commit adds support into the CommonJS and UMD reflection hosts for this style of re-export syntax. PR Close #36989 --- .../ngcc/src/host/commonjs_host.ts | 22 +++- .../ngcc/src/host/commonjs_umd_utils.ts | 81 ++++++++++++ .../compiler-cli/ngcc/src/host/umd_host.ts | 119 +++++++++++++----- .../ngcc/test/host/commonjs_host_spec.ts | 25 +++- .../ngcc/test/host/umd_host_spec.ts | 35 +++++- 5 files changed, 241 insertions(+), 41 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts index 00771a60fa..2df636ee60 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_host.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_host.ts @@ -14,7 +14,7 @@ import {Logger} from '../logging/logger'; import {BundleProgram} from '../packages/bundle_program'; import {FactoryMap, isDefined} from '../utils'; -import {ExportDeclaration, ExportStatement, findNamespaceOfIdentifier, findRequireCallReference, isExportStatement, isRequireCall, isWildcardReexportStatement, RequireCall, WildcardReexportStatement} from './commonjs_umd_utils'; +import {DefinePropertyReexportStatement, ExportDeclaration, ExportStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, RequireCall, WildcardReexportStatement} from './commonjs_umd_utils'; import {Esm5ReflectionHost} from './esm5_host'; import {NgccClassSymbol} from './ngcc_host'; @@ -106,6 +106,11 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { for (const reexport of reexports) { moduleMap.set(reexport.name, reexport.declaration); } + } else if (isDefinePropertyReexportStatement(statement)) { + const exportDeclaration = this.extractCommonJsDefinePropertyExportDeclaration(statement); + if (exportDeclaration !== null) { + moduleMap.set(exportDeclaration.name, exportDeclaration.declaration); + } } } return moduleMap; @@ -139,7 +144,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { return []; } - const viaModule = importPath.startsWith('./') ? null : importPath; + const viaModule = isExternalImport(importPath) ? importPath : null; const reexports: ExportDeclaration[] = []; importedExports.forEach((declaration, name) => { if (viaModule !== null && declaration.viaModule === null) { @@ -150,6 +155,17 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { return reexports; } + private extractCommonJsDefinePropertyExportDeclaration( + statement: DefinePropertyReexportStatement): ExportDeclaration|null { + const args = statement.expression.arguments; + const name = args[1].text; + const getterFnExpression = extractGetterFnExpression(statement); + if (getterFnExpression === null) { + return null; + } + return this.extractCommonJsExportDeclaration(name, getterFnExpression); + } + private findCommonJsImport(id: ts.Identifier): RequireCall|null { // Is `id` a namespaced property access, e.g. `Directive` in `core.Directive`? // If so capture the symbol of the namespace, e.g. `core`. @@ -187,7 +203,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost { if (module === undefined) { return null; } - const viaModule = importPath.startsWith('./') ? null : importPath; + const viaModule = isExternalImport(importPath) ? importPath : null; return {node: module, known: null, viaModule, identity: null}; } diff --git a/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts b/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts index c49113aaf2..a9e678fc4d 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts @@ -45,6 +45,20 @@ export interface WildcardReexportStatement extends ts.ExpressionStatement { expression: ts.CallExpression; } +/** + * A CommonJS or UMD re-export statement using an `Object.defineProperty()` call. + * For example: + * + * ``` + * Object.defineProperty(exports, "", + * { enumerable: true, get: function () { return ; } }); + * ``` + */ +export interface DefinePropertyReexportStatement extends ts.ExpressionStatement { + expression: ts.CallExpression& + {arguments: [ts.Identifier, ts.StringLiteral, ts.ObjectLiteralExpression]}; +} + export interface RequireCall extends ts.CallExpression { arguments: ts.CallExpression['arguments']&[ts.StringLiteral]; } @@ -133,6 +147,69 @@ export function isWildcardReexportStatement(stmt: ts.Statement): stmt is Wildcar return stmt.expression.arguments.length > 0; } + +/** + * Check whether the statement is a re-export of the form: + * + * ``` + * Object.defineProperty(exports, "", + * { enumerable: true, get: function () { return ; } }); + * ``` + */ +export function isDefinePropertyReexportStatement(stmt: ts.Statement): + stmt is DefinePropertyReexportStatement { + if (!ts.isExpressionStatement(stmt) || !ts.isCallExpression(stmt.expression)) { + return false; + } + + // Check for Object.defineProperty + if (!ts.isPropertyAccessExpression(stmt.expression.expression) || + !ts.isIdentifier(stmt.expression.expression.expression) || + stmt.expression.expression.expression.text !== 'Object' || + !ts.isIdentifier(stmt.expression.expression.name) || + stmt.expression.expression.name.text !== 'defineProperty') { + return false; + } + + const args = stmt.expression.arguments; + if (args.length !== 3) { + return false; + } + const exportsObject = args[0]; + if (!ts.isIdentifier(exportsObject) || exportsObject.text !== 'exports') { + return false; + } + + const propertyKey = args[1]; + if (!ts.isStringLiteral(propertyKey)) { + return false; + } + + const propertyDescriptor = args[2]; + if (!ts.isObjectLiteralExpression(propertyDescriptor)) { + return false; + } + + return (propertyDescriptor.properties.some( + prop => prop.name !== undefined && ts.isIdentifier(prop.name) && prop.name.text === 'get')); +} + +export function extractGetterFnExpression(statement: DefinePropertyReexportStatement): + ts.Expression|null { + const args = statement.expression.arguments; + const getterFn = args[2].properties.find( + prop => prop.name !== undefined && ts.isIdentifier(prop.name) && prop.name.text === 'get'); + if (getterFn === undefined || !ts.isPropertyAssignment(getterFn) || + !ts.isFunctionExpression(getterFn.initializer)) { + return null; + } + const returnStatement = getterFn.initializer.body.statements[0]; + if (!ts.isReturnStatement(returnStatement) || returnStatement.expression === undefined) { + return null; + } + return returnStatement.expression; +} + /** * Check whether the specified `ts.Node` represents a `require()` call, i.e. an call expression of * the form: `require('')` @@ -142,3 +219,7 @@ export function isRequireCall(node: ts.Node): node is RequireCall { node.expression.text === 'require' && node.arguments.length === 1 && ts.isStringLiteral(node.arguments[0]); } + +export function isExternalImport(path: string): boolean { + return !/^\.\.?(\/|$)/.test(path); +} \ No newline at end of file diff --git a/packages/compiler-cli/ngcc/src/host/umd_host.ts b/packages/compiler-cli/ngcc/src/host/umd_host.ts index 234d220e88..c45bf2b196 100644 --- a/packages/compiler-cli/ngcc/src/host/umd_host.ts +++ b/packages/compiler-cli/ngcc/src/host/umd_host.ts @@ -14,7 +14,7 @@ import {Logger} from '../logging/logger'; import {BundleProgram} from '../packages/bundle_program'; import {FactoryMap, getTsHelperFnFromIdentifier, stripExtension} from '../utils'; -import {ExportDeclaration, ExportStatement, findNamespaceOfIdentifier, findRequireCallReference, isExportStatement, isRequireCall, isWildcardReexportStatement, WildcardReexportStatement} from './commonjs_umd_utils'; +import {DefinePropertyReexportStatement, ExportDeclaration, ExportStatement, extractGetterFnExpression, findNamespaceOfIdentifier, findRequireCallReference, isDefinePropertyReexportStatement, isExportStatement, isExternalImport, isRequireCall, isWildcardReexportStatement, WildcardReexportStatement} from './commonjs_umd_utils'; import {Esm5ReflectionHost} from './esm5_host'; import {stripParentheses} from './utils'; @@ -44,7 +44,8 @@ export class UmdReflectionHost extends Esm5ReflectionHost { } getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null { - return this.getUmdImportedDeclaration(id) || super.getDeclarationOfIdentifier(id); + return this.getUmdModuleDeclaration(id) || this.getUmdDeclaration(id) || + super.getDeclarationOfIdentifier(id); } getExportsOfModule(module: ts.Node): Map|null { @@ -90,13 +91,18 @@ export class UmdReflectionHost extends Esm5ReflectionHost { const moduleMap = new Map(); for (const statement of this.getModuleStatements(sourceFile)) { if (isExportStatement(statement)) { - const exportDeclaration = this.extractUmdExportDeclaration(statement); + const exportDeclaration = this.extractBasicUmdExportDeclaration(statement); moduleMap.set(exportDeclaration.name, exportDeclaration.declaration); } else if (isWildcardReexportStatement(statement)) { const reexports = this.extractUmdWildcardReexports(statement, sourceFile); for (const reexport of reexports) { moduleMap.set(reexport.name, reexport.declaration); } + } else if (isDefinePropertyReexportStatement(statement)) { + const exportDeclaration = this.extractUmdDefinePropertyExportDeclaration(statement); + if (exportDeclaration !== null) { + moduleMap.set(exportDeclaration.name, exportDeclaration.declaration); + } } } return moduleMap; @@ -126,23 +132,10 @@ export class UmdReflectionHost extends Esm5ReflectionHost { return importPath; } - private extractUmdExportDeclaration(statement: ExportStatement): ExportDeclaration { - const exportExpression = statement.expression.right; - const declaration = this.getDeclarationOfExpression(exportExpression); + private extractBasicUmdExportDeclaration(statement: ExportStatement): ExportDeclaration { const name = statement.expression.left.name.text; - if (declaration !== null) { - return {name, declaration}; - } else { - return { - name, - declaration: { - node: null, - known: null, - expression: exportExpression, - viaModule: null, - }, - }; - } + const exportExpression = statement.expression.right; + return this.extractUmdExportDeclaration(name, exportExpression); } private extractUmdWildcardReexports( @@ -192,6 +185,28 @@ export class UmdReflectionHost extends Esm5ReflectionHost { return reexports; } + private extractUmdDefinePropertyExportDeclaration(statement: DefinePropertyReexportStatement): + ExportDeclaration|null { + const args = statement.expression.arguments; + const name = args[1].text; + const getterFnExpression = extractGetterFnExpression(statement); + if (getterFnExpression === null) { + return null; + } + return this.extractUmdExportDeclaration(name, getterFnExpression); + } + + private extractUmdExportDeclaration(name: string, expression: ts.Expression): ExportDeclaration { + const declaration = this.getDeclarationOfExpression(expression); + if (declaration !== null) { + return {name, declaration}; + } + return { + name, + declaration: {node: null, known: null, expression, viaModule: null}, + }; + } + /** * Is the identifier a parameter on a UMD factory function, e.g. `function factory(this, core)`? * If so then return its declaration. @@ -202,25 +217,67 @@ export class UmdReflectionHost extends Esm5ReflectionHost { return declaration && ts.isParameter(declaration) ? declaration : null; } - private getUmdImportedDeclaration(id: ts.Identifier): Declaration|null { - const importInfo = this.getImportOfIdentifier(id); - if (importInfo === null) { + private getUmdDeclaration(id: ts.Identifier): Declaration|null { + const nsIdentifier = findNamespaceOfIdentifier(id); + if (nsIdentifier === null) { + return null; + } + const moduleDeclaration = this.getUmdModuleDeclaration(nsIdentifier); + if (moduleDeclaration === null || moduleDeclaration.node === null || + !ts.isSourceFile(moduleDeclaration.node)) { return null; } - const importedFile = this.resolveModuleName(importInfo.from, id.getSourceFile()); - if (importedFile === undefined) { + const moduleExports = this.getExportsOfModule(moduleDeclaration.node); + if (moduleExports === null) { return null; } - // We need to add the `viaModule` because the `getExportsOfModule()` call + // We need to compute the `viaModule` because the `getExportsOfModule()` call // did not know that we were importing the declaration. - return { - node: importedFile, - known: getTsHelperFnFromIdentifier(id), - viaModule: importInfo.from, - identity: null - }; + const declaration = moduleExports.get(id.text)!; + + if (!moduleExports.has(id.text)) { + return null; + } + + // We need to compute the `viaModule` because the `getExportsOfModule()` call + // did not know that we were importing the declaration. + const viaModule = + declaration.viaModule === null ? moduleDeclaration.viaModule : declaration.viaModule; + + return {...declaration, viaModule, known: getTsHelperFnFromIdentifier(id)}; + } + + private getUmdModuleDeclaration(id: ts.Identifier): Declaration|null { + const importPath = this.getImportPathFromParameter(id) || this.getImportPathFromRequireCall(id); + if (importPath === null) { + return null; + } + + const module = this.resolveModuleName(importPath, id.getSourceFile()); + if (module === undefined) { + return null; + } + + const viaModule = isExternalImport(importPath) ? importPath : null; + return {node: module, viaModule, known: null, identity: null}; + } + + private getImportPathFromParameter(id: ts.Identifier): string|null { + const importParameter = this.findUmdImportParameter(id); + if (importParameter === null) { + return null; + } + return this.getUmdImportPath(importParameter); + } + + private getImportPathFromRequireCall(id: ts.Identifier): string|null { + const requireCall = findRequireCallReference(id, this.checker); + if (requireCall === null) { + return null; + } + return requireCall.arguments[0].text; } private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts index 97565d497b..4a05833409 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -517,8 +517,8 @@ var c = file_a.a; `var b_module = require('./b_module');\n` + `var xtra_module = require('./xtra_module');\n` + `var wildcard_reexports_emitted_helpers = require('./wildcard_reexports_emitted_helpers');\n` + - `var wildcard_reexports_imported_helpers = require('./wildcard_reexports_imported_helpers');\n`, - + `var wildcard_reexports_imported_helpers = require('./wildcard_reexports_imported_helpers');\n` + + `var define_property_reexports = require('./define_property_reexports');\n` }, { name: _('/a_module.js'), @@ -570,6 +570,11 @@ var c = file_a.a; `tslib_1.__exportStar(b_module, exports);\n` + `tslib_1.__exportStar(require("./xtra_module"), exports);\n`, }, + { + name: _('/define_property_reexports.js'), + contents: `var moduleA = require("./a_module");\n` + + `Object.defineProperty(exports, "newA", { enumerable: true, get: function () { return moduleA.a; } });`, + } ]; FUNCTION_BODY_FILE = { @@ -2180,6 +2185,22 @@ exports.MissingClass2 = MissingClass2; ['__unknownHelper', null], ]); }); + + it('should define property exports from a module', () => { + loadFakeCore(getFileSystem()); + loadTestFiles(EXPORTS_FILES); + const bundle = makeTestBundleProgram(_('/index.js')); + const host = + createHost(bundle, new CommonJsReflectionHost(new MockLogger(), false, bundle)); + const file = getSourceFileOrError(bundle.program, _('/define_property_reexports.js')); + const exportDeclarations = host.getExportsOfModule(file); + expect(exportDeclarations).not.toBe(null); + expect(Array.from(exportDeclarations!.entries()) + .map(entry => [entry[0], entry[1].node!.getText(), entry[1].viaModule])) + .toEqual([ + ['newA', `a = 'a'`, null], + ]); + }); }); describe('getClassSymbol()', () => { diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts index 0cd2dbd33b..94e4fe3e09 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -571,10 +571,10 @@ runInEachFileSystem(() => { { name: _('/index.js'), contents: `(function (global, factory) {\n` + - ` typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('./a_module'), require('./b_module'), require('./wildcard_reexports'), require('./wildcard_reexports_imported_helpers'), require('./wildcard_reexports_with_require')) :\n` + - ` typeof define === 'function' && define.amd ? define('index', ['exports', './a_module', './b_module', './wildcard_reexports', './wildcard_reexports_imported_helpers', './wildcard_reexports_with_require'], factory) :\n` + - ` (factory(global.index, global.a_module, global.b_module, global.wildcard_reexports, global.wildcard_reexports_imported_helpers, global.wildcard_reexports_with_require));\n` + - `}(this, (function (exports, a_module, b_module, wildcard_reexports, wildcard_reexports_imported_helpers, wildcard_reexports_with_require) { 'use strict';\n` + + ` typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('./a_module'), require('./b_module'), require('./wildcard_reexports'), require('./wildcard_reexports_imported_helpers'), require('./wildcard_reexports_with_require'), require('./define_property_reexports')) :\n` + + ` typeof define === 'function' && define.amd ? define('index', ['exports', './a_module', './b_module', './wildcard_reexports', './wildcard_reexports_imported_helpers', './wildcard_reexports_with_require', './define_property_reexports'], factory) :\n` + + ` (factory(global.index, global.a_module, global.b_module, global.wildcard_reexports, global.wildcard_reexports_imported_helpers, global.wildcard_reexports_with_require, global.define_property_reexports));\n` + + `}(this, (function (exports, a_module, b_module, wildcard_reexports, wildcard_reexports_imported_helpers, wildcard_reexports_with_require, define_property_reexports) { 'use strict';\n` + `})));\n` }, { @@ -663,7 +663,17 @@ runInEachFileSystem(() => { ` __export(b_module);\n` + ` __export(require('./xtra_module'));\n` + `})));\n`, - } + }, + { + name: _('/define_property_reexports.js'), + contents: `(function (global, factory) {\n` + + ` typeof exports === 'object' && typeof module !== 'undefined' ? factory(require, exports) :\n` + + ` typeof define === 'function' && define.amd ? define('define_property_reexports', ['require', 'exports'], factory);\n` + + `}(this, (function (require, exports) { 'use strict';\n` + + `var moduleA = require("./a_module");\n` + + `Object.defineProperty(exports, "newA", { enumerable: true, get: function () { return moduleA.a; } });\n` + + `})));`, + }, ]; FUNCTION_BODY_FILE = { @@ -2285,6 +2295,21 @@ runInEachFileSystem(() => { ['__unknownHelper', null], ]); }); + + it('should define property exports from a module', () => { + loadFakeCore(getFileSystem()); + loadTestFiles(EXPORTS_FILES); + const bundle = makeTestBundleProgram(_('/index.js')); + const host = createHost(bundle, new UmdReflectionHost(new MockLogger(), false, bundle)); + const file = getSourceFileOrError(bundle.program, _('/define_property_reexports.js')); + const exportDeclarations = host.getExportsOfModule(file); + expect(exportDeclarations).not.toBe(null); + expect(Array.from(exportDeclarations!.entries()) + .map(entry => [entry[0], entry[1].node!.getText(), entry[1].viaModule])) + .toEqual([ + ['newA', `a = 'a'`, null], + ]); + }); }); describe('getClassSymbol()', () => {