From c3271ac22ae48281a8ce69052434390da754e6e3 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 5 Dec 2019 21:02:57 +0200 Subject: [PATCH] fix(ngcc): recognize re-exports with imported TS helpers in CommonJS and UMD (#34527) Previously, the `CommonJsReflectionHost` and `UmdReflectionHost` would only recognize re-exports of the form `__export(...)`. This is what re-exports look like, when the TypeScript helpers are emitted inline (i.e. when compiling with the default [TypeScript compiler options][1] that include `noEmitHelpers: false` and `importHelpers: false`). However, when compiling with `importHelpers: true` and [tslib][2] (which is the recommended way for optimized bundles), the re-exports will look like: `tslib_1.__exportStar(..., exports)` These types of re-exports were previously not recognized by the CommonJS/UMD `ReflectionHost`s and thus ignored. This commit fixes this by ensuring both re-export formats are recognized. [1]: https://www.typescriptlang.org/docs/handbook/compiler-options.html [2]: https://www.npmjs.com/package/tslib PR Close #34527 --- .../ngcc/src/host/commonjs_umd_utils.ts | 58 +++++++++++++++-- .../ngcc/test/host/commonjs_host_spec.ts | 64 +++++++++++++++---- .../ngcc/test/host/umd_host_spec.ts | 51 +++++++++++++-- 3 files changed, 152 insertions(+), 21 deletions(-) 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 27bc1597b8..86ddc95503 100644 --- a/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts +++ b/packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts @@ -19,6 +19,21 @@ export interface ExportStatement extends ts.ExpressionStatement { expression: ts.BinaryExpression&{left: ts.PropertyAccessExpression & {expression: ts.Identifier}}; } +/** + * A CommonJS or UMD re-export statement. + * + * In CommonJS/UMD, re-export statements can have several forms (depending, for example, on whether + * the TypeScript helpers are imported or emitted inline). The expression can have one of the + * following forms: + * - `__export(firstArg)` + * - `__exportStar(firstArg)` + * - `tslib.__export(firstArg, exports)` + * - `tslib.__exportStar(firstArg, exports)` + * + * In all cases, we only care about `firstApp`, which is the first argument of the re-export call + * expression and can be either a `require('...')` call or an identifier (initialized via a + * `require('...')` call). + */ export interface ReexportStatement extends ts.ExpressionStatement { expression: ts.CallExpression; } export interface RequireCall extends ts.CallExpression { @@ -66,12 +81,47 @@ export function isExportStatement(stmt: ts.Statement): stmt is ExportStatement { /** * Check whether the specified `ts.Statement` is a re-export statement, i.e. an expression statement - * of the form: `__export()` + * of one of the following forms: + * - `__export()` + * - `__exportStar()` + * - `tslib.__export(, exports)` + * - `tslib.__exportStar(, exports)` */ 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; + // Ensure it is a call expression statement. + if (!ts.isExpressionStatement(stmt) || !ts.isCallExpression(stmt.expression)) { + return false; + } + + // Get the called function identifier. + // NOTE: Currently, it seems that `__export()` is used when emitting helpers inline and + // `__exportStar()` when importing them + // ([source](https://github.com/microsoft/TypeScript/blob/d7c83f023/src/compiler/transformers/module/module.ts#L1796-L1797)). + // So, theoretically, we only care about the formats `__export()` and + // `tslib.__exportStar(, exports)`. + // The current implementation accepts the other two formats (`__exportStar(...)` and + // `tslib.__export(...)`) as well to be more future-proof (given that it is unlikely that + // they will introduce false positives). + let fnName: string|null = null; + if (ts.isIdentifier(stmt.expression.expression)) { + // Statement of the form `someFn(...)`. + fnName = stmt.expression.expression.text; + } else if ( + ts.isPropertyAccessExpression(stmt.expression.expression) && + ts.isIdentifier(stmt.expression.expression.name)) { + // Statement of the form `tslib.someFn(...)`. + fnName = stmt.expression.expression.name.text; + } + + // Ensure the called function is either `__export()` or `__exportStar()`. + if ((fnName !== '__export') && (fnName !== '__exportStar')) { + return false; + } + + // Ensure there is at least one argument. + // (The first argument is the exported thing and there will be a second `exports` argument in the + // case of imported helpers). + return stmt.expression.arguments.length > 0; } /** 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 721cab1565..cba513de17 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -118,7 +118,7 @@ exports.SomeDirective = SomeDirective; contents: ` var core = require('@angular/core'); var CtorDecoratedAsArray = (function() { - function CtorDecoratedAsArray(arg1) { + function CtorDecoratedAsArray(arg1) { } CtorDecoratedAsArray.ctorParameters = [{ type: ParamType, decorators: [{ type: Inject },] }]; return CtorDecoratedAsArray; @@ -510,7 +510,8 @@ var c = file_a.a; var a_module = require('./a_module'); var b_module = require('./b_module'); var xtra_module = require('./xtra_module'); - var wildcard_reexports = require('./wildcard_reexports'); + var wildcard_reexports_emitted_helpers = require('./wildcard_reexports_emitted_helpers'); + var wildcard_reexports_imported_helpers = require('./wildcard_reexports_imported_helpers'); ` }, { @@ -552,15 +553,24 @@ exports.xtra2 = xtra2; `, }, { - name: _('/wildcard_reexports.js'), + name: _('/wildcard_reexports_emitted_helpers.js'), contents: ` - function __export(m) { - for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p]; - } - var b_module = require("./b_module"); - __export(b_module); - __export(require("./xtra_module")); - ` +function __export(m) { + for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p]; +} +var b_module = require("./b_module"); +__export(b_module); +__export(require("./xtra_module")); +`, + }, + { + name: _('/wildcard_reexports_imported_helpers.js'), + contents: ` +var tslib_1 = require("tslib"); +var b_module = require("./b_module"); +tslib_1.__exportStar(b_module, exports); +tslib_1.__exportStar(require("./xtra_module"), exports); +`, }, ]; @@ -1763,12 +1773,42 @@ exports.ExternalModule = ExternalModule; ]); }); - it('should handle wildcard re-exports of other modules', () => { + it('should handle wildcard re-exports of other modules (with emitted helpers)', () => { loadFakeCore(getFileSystem()); loadTestFiles(EXPORTS_FILES); const bundle = makeTestBundleProgram(_('/index.js')); const host = new CommonJsReflectionHost(new MockLogger(), false, bundle); - const file = getSourceFileOrError(bundle.program, _('/wildcard_reexports.js')); + const file = + getSourceFileOrError(bundle.program, _('/wildcard_reexports_emitted_helpers.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([ + ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], + ['a', `a = 'a'`, _('/b_module')], + ['b', `b = a_module.a`, _('/b_module')], + ['c', `a = 'a'`, _('/b_module')], + ['d', `b = a_module.a`, _('/b_module')], + ['e', `e = 'e'`, _('/b_module')], + ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], + [ + 'SomeClass', + `SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n}())`, + _('/b_module') + ], + ['xtra1', `xtra1 = 'xtra1'`, _('/xtra_module')], + ['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')], + ]); + }); + + it('should handle wildcard re-exports of other modules (with imported helpers)', () => { + loadFakeCore(getFileSystem()); + loadTestFiles(EXPORTS_FILES); + const bundle = makeTestBundleProgram(_('/index.js')); + const host = new CommonJsReflectionHost(new MockLogger(), false, bundle); + const file = + getSourceFileOrError(bundle.program, _('/wildcard_reexports_imported_helpers.js')); const exportDeclarations = host.getExportsOfModule(file); expect(exportDeclarations).not.toBe(null); expect(Array.from(exportDeclarations !.entries()) 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 f1f2dd3a78..9ecf3b4042 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -564,10 +564,10 @@ runInEachFileSystem(() => { name: _('/index.js'), contents: ` (function (global, factory) { - typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('./a_module'), require('./b_module'), require('./wildcard_reexports'), require('./wildcard_reexports_with_require')) : - typeof define === 'function' && define.amd ? define('index', ['exports', './a_module', './b_module', './wildcard_reexports', './wildcard_reexports_with_require'], factory) : - (factory(global.index, global.a_module, global.b_module, global.wildcard_reexports, global.wildcard_reexports_with_require)); - }(this, (function (exports, a_module, b_module, wildcard_reexports, wildcard_reexports_with_require) { 'use strict'; + 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')) : + typeof define === 'function' && define.amd ? define('index', ['exports', './a_module', './b_module', './wildcard_reexports', './wildcard_reexports_imported_helpers', './wildcard_reexports_with_require'], factory) : + (factory(global.index, global.a_module, global.b_module, global.wildcard_reexports, global.wildcard_reexports_imported_helpers, global.wildcard_reexports_with_require)); + }(this, (function (exports, a_module, b_module, wildcard_reexports, wildcard_reexports_imported_helpers, wildcard_reexports_with_require) { 'use strict'; }))); ` }, @@ -635,6 +635,18 @@ runInEachFileSystem(() => { } __export(b_module); __export(xtra_module); +})));`, + }, + { + name: _('/wildcard_reexports_imported_helpers.js'), + contents: ` +(function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('tslib'), require('./b_module'), require('./xtra_module')) : + typeof define === 'function' && define.amd ? define('wildcard_reexports', ['exports', 'tslib', './b_module', './xtra_module'], factory) : + (factory(global.wildcard_reexports_imported_helpers, tslib, b_module, xtra_module)); +}(this, (function (exports, tslib, b_module, xtra_module) { 'use strict'; + tslib.__exportStar(b_module, exports); + tslib.__exportStar(xtra_module, exports); })));`, }, { @@ -1947,7 +1959,7 @@ runInEachFileSystem(() => { expect(classSymbol !.implementation.valueDeclaration).toBe(node); }); - it('should handle wildcard re-exports of other modules', () => { + it('should handle wildcard re-exports of other modules (with emitted helpers)', () => { loadFakeCore(getFileSystem()); loadTestFiles(EXPORTS_FILES); const bundle = makeTestBundleProgram(_('/index.js')); @@ -1975,6 +1987,35 @@ runInEachFileSystem(() => { ]); }); + it('should handle wildcard re-exports of other modules (with imported helpers)', () => { + loadFakeCore(getFileSystem()); + loadTestFiles(EXPORTS_FILES); + const bundle = makeTestBundleProgram(_('/index.js')); + const host = new UmdReflectionHost(new MockLogger(), false, bundle); + const file = + getSourceFileOrError(bundle.program, _('/wildcard_reexports_imported_helpers.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([ + ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], + ['a', `a = 'a'`, _('/b_module')], + ['b', `b = a_module.a`, _('/b_module')], + ['c', `a = 'a'`, _('/b_module')], + ['d', `b = a_module.a`, _('/b_module')], + ['e', `e = 'e'`, _('/b_module')], + ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')], + [ + 'SomeClass', + `SomeClass = (function() {\n function SomeClass() {}\n return SomeClass;\n }())`, + _('/b_module') + ], + ['xtra1', `xtra1 = 'xtra1'`, _('/xtra_module')], + ['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')], + ]); + }); + it('should handle wildcard re-exports of other modules using `require()` calls', () => { loadFakeCore(getFileSystem()); loadTestFiles(EXPORTS_FILES);