From 81a1117681267835530e00a025a3d091715b7073 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Mon, 8 Feb 2021 00:04:03 -0800 Subject: [PATCH] fix #758: subtle circular dependency issue --- CHANGELOG.md | 6 ++- internal/bundler/linker.go | 41 ++++++++++++++++--- internal/bundler/snapshots/snapshots_dce.txt | 3 ++ .../bundler/snapshots/snapshots_default.txt | 40 ++++++++++++++---- .../snapshots/snapshots_importstar.txt | 16 ++++++++ .../snapshots/snapshots_importstar_ts.txt | 1 + .../snapshots/snapshots_packagejson.txt | 1 + .../bundler/snapshots/snapshots_splitting.txt | 14 ++++--- internal/js_ast/js_ast.go | 2 +- internal/runtime/runtime.go | 4 +- scripts/end-to-end-tests.js | 38 +++++++++++++++++ 11 files changed, 140 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 839fad03a0e..0a9cdb525b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,11 @@ Previously recursive symlinks (a symlink that points to another symlink) were an unhandled case in the path resolution algorithm. Now these cases should be supported up to a depth of 256 symlinks. This means esbuild's path resolution should now work with multi-level `yarn link` scenarios. -## 0.8.42 +* Fix subtle circular dependency issue ([#758](https://github.com/evanw/esbuild/issues/758)) + + If esbuild is used to transform TypeScript to JavaScript without bundling (i.e. each file is transformed individually), the output format is CommonJS, and the original TypeScript code contains an import cycle where at least one of the links in the cycle is an `export * as` re-export statement, there could be certain situations where evaluating the transformed code results in an import being `undefined`. This is caused by the `__esModule` marker being added after the call to `require()` for the first transformed re-export statement. The fix was to move the marker to before the first call to `require()`. The `__esModule` marker is a convention from Babel that esbuild reuses which marks a module as being originally in the ECMAScript module format instead of the CommonJS module format. + +* ## 0.8.42 * Fix crash with block-level function declaration and `--keep-names` ([#755](https://github.com/evanw/esbuild/issues/755)) diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index b5ab3751c0b..3dceae6840c 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -173,7 +173,8 @@ type fileMeta struct { // This is set when we need to pull in the "__export" symbol in to the part // at "nsExportPartIndex". This can't be done in "createExportsForFile" // because of concurrent map hazards. Instead, it must be done later. - needsExportSymbolFromRuntime bool + needsExportSymbolFromRuntime bool + needsMarkAsModuleSymbolFromRuntime bool // The index of the automatically-generated part used to represent the // CommonJS wrapper. This part is empty and is only useful for tree shaking @@ -1105,7 +1106,7 @@ func (c *linkerContext) scanImportsAndExports() { // // In that case the module *is* considered a CommonJS module because // the namespace object must be created. - if record.ContainsImportStar && !otherRepr.ast.HasES6Syntax() && !otherRepr.ast.HasLazyExport { + if record.ContainsImportStar && !otherRepr.ast.HasES6ImportsOrExports() && !otherRepr.ast.HasLazyExport { otherRepr.meta.cjsStyleExports = true } @@ -1318,11 +1319,17 @@ func (c *linkerContext) scanImportsAndExports() { // Include the "__export" symbol from the runtime if it was used in the // previous step. The previous step can't do this because it's running in // parallel and can't safely mutate the "importsToBind" map of another file. - if repr.meta.needsExportSymbolFromRuntime { + if repr.meta.needsExportSymbolFromRuntime || repr.meta.needsMarkAsModuleSymbolFromRuntime { runtimeRepr := c.files[runtime.SourceIndex].repr.(*reprJS) - exportRef := runtimeRepr.ast.ModuleScope.Members["__export"].Ref exportPart := &repr.ast.Parts[repr.meta.nsExportPartIndex] - c.generateUseOfSymbolForInclude(exportPart, &repr.meta, 1, exportRef, runtime.SourceIndex) + if repr.meta.needsExportSymbolFromRuntime { + exportRef := runtimeRepr.ast.ModuleScope.Members["__export"].Ref + c.generateUseOfSymbolForInclude(exportPart, &repr.meta, 1, exportRef, runtime.SourceIndex) + } + if repr.meta.needsMarkAsModuleSymbolFromRuntime { + exportRef := runtimeRepr.ast.ModuleScope.Members["__markAsModule"].Ref + c.generateUseOfSymbolForInclude(exportPart, &repr.meta, 1, exportRef, runtime.SourceIndex) + } } for importRef, importToBind := range repr.meta.importsToBind { @@ -1633,6 +1640,28 @@ func (c *linkerContext) createExportsForFile(sourceIndex uint32) { }) } + // If this file was originally ESM but is now in CommonJS, add a call to + // "__markAsModule" which sets the "__esModule" property to true. This must + // be done before any to "require()" or circular imports of multiple modules + // that have been each converted from ESM to CommonJS may not work correctly. + if repr.ast.HasES6Exports && (repr.meta.cjsStyleExports || (file.isEntryPoint && c.options.OutputFormat == config.FormatCommonJS)) { + runtimeRepr := c.files[runtime.SourceIndex].repr.(*reprJS) + markAsModuleRef := runtimeRepr.ast.ModuleScope.Members["__markAsModule"].Ref + nsExportStmts = append(nsExportStmts, js_ast.Stmt{Data: &js_ast.SExpr{Value: js_ast.Expr{Data: &js_ast.ECall{ + Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: markAsModuleRef}}, + Args: []js_ast.Expr{{Data: &js_ast.EIdentifier{Ref: repr.ast.ExportsRef}}}, + }}}}) + + // Make sure this file depends on the "__markAsModule" symbol + for _, partIndex := range runtimeRepr.ast.TopLevelSymbolToParts[markAsModuleRef] { + dep := partRef{sourceIndex: runtime.SourceIndex, partIndex: partIndex} + nsExportNonLocalDependencies = append(nsExportNonLocalDependencies, dep) + } + + // Pull in the "__markAsModule" symbol later + repr.meta.needsMarkAsModuleSymbolFromRuntime = true + } + // "__export(exports, { foo: () => foo })" exportRef := js_ast.InvalidRef if len(properties) > 0 { @@ -2201,7 +2230,7 @@ func (c *linkerContext) advanceImportTracker(tracker importTracker) (importTrack // Is this a named import of a file without any exports? otherRepr := c.files[otherSourceIndex].repr.(*reprJS) - if namedImport.Alias != "*" && !otherRepr.ast.UsesCommonJSExports() && !otherRepr.ast.HasES6Syntax() && !otherRepr.ast.HasLazyExport { + if namedImport.Alias != "*" && !otherRepr.ast.UsesCommonJSExports() && !otherRepr.ast.HasES6ImportsOrExports() && !otherRepr.ast.HasLazyExport { // Just warn about it and replace the import with "undefined" return importTracker{sourceIndex: otherSourceIndex, importRef: js_ast.InvalidRef}, importCommonJSWithoutExports, nil } diff --git a/internal/bundler/snapshots/snapshots_dce.txt b/internal/bundler/snapshots/snapshots_dce.txt index a2073340546..4b9a0cd61aa 100644 --- a/internal/bundler/snapshots/snapshots_dce.txt +++ b/internal/bundler/snapshots/snapshots_dce.txt @@ -143,6 +143,7 @@ TestPackageJsonSideEffectsArrayKeepMainImplicitMain ---------- /out.js ---------- // Users/user/project/node_modules/demo-pkg/index-main.js var require_index_main = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo2 }); @@ -185,6 +186,7 @@ TestPackageJsonSideEffectsArrayKeepModuleImplicitMain ---------- /out.js ---------- // Users/user/project/node_modules/demo-pkg/index-main.js var require_index_main = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo }); @@ -246,6 +248,7 @@ TestPackageJsonSideEffectsFalseKeepBareImportAndRequireES6 ---------- /out.js ---------- // Users/user/project/node_modules/demo-pkg/index.js var require_demo_pkg = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo }); diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index 9d0bc3b9286..4ff138d604a 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -236,6 +236,7 @@ TestCommonJSFromES6 ---------- /out.js ---------- // foo.js var require_foo = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo2 }); @@ -246,6 +247,7 @@ var require_foo = __commonJS((exports) => { // bar.js var require_bar = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { bar: () => bar2 }); @@ -473,6 +475,7 @@ TestExportFSNodeInCommonJSModule import * as fs from "fs"; import {readFileSync} from "fs"; var require_entry = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { fs: () => fs, readFileSync: () => readFileSync @@ -486,6 +489,7 @@ TestExportFormsCommonJS ---------- /out.js ---------- // commonjs.js var require_commonjs = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { C: () => Class, Class: () => Class, @@ -509,6 +513,7 @@ var require_commonjs = __commonJS((exports) => { // c.js var require_c = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => c_default2 }); @@ -519,6 +524,7 @@ var require_c = __commonJS((exports) => { // d.js var require_d = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => d_default }); @@ -530,6 +536,7 @@ var require_d = __commonJS((exports) => { // e.js var require_e = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => e_default }); @@ -539,6 +546,7 @@ var require_e = __commonJS((exports) => { // f.js var require_f = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => foo }); @@ -549,6 +557,7 @@ var require_f = __commonJS((exports) => { // g.js var require_g = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => g_default }); @@ -558,6 +567,7 @@ var require_g = __commonJS((exports) => { // h.js var require_h = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => foo }); @@ -698,6 +708,7 @@ export default class o { TestExportWildcardFSNodeCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __exportStar(exports, __toModule(require("fs"))); ================================================================================ @@ -732,6 +743,7 @@ TestExternalES6ConvertedToCommonJS // a.js import * as ns from "x"; var require_a = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { ns: () => ns }); @@ -740,6 +752,7 @@ var require_a = __commonJS((exports) => { // b.js import * as ns2 from "x"; var require_b = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { ns: () => ns2 }); @@ -748,6 +761,7 @@ var require_b = __commonJS((exports) => { // c.js import * as ns3 from "x"; var require_c = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { ns: () => ns3 }); @@ -756,6 +770,7 @@ var require_c = __commonJS((exports) => { // d.js import {ns as ns4} from "x"; var require_d = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { ns: () => ns4 }); @@ -764,6 +779,7 @@ var require_d = __commonJS((exports) => { // e.js import * as x_star from "x"; var require_e = __commonJS((exports) => { + __markAsModule(exports); __exportStar(exports, x_star); }); @@ -1418,21 +1434,21 @@ Promise.resolve().then(function(){return __toModule(require("foo"))});Promise.re TestMinifiedExportsAndModuleFormatCommonJS ---------- /out.js ---------- // foo/test.js -var t = {}; -f(t, { - foo: () => l +var o = {}; +s(o, { + foo: () => p }); -var l = 123; +var p = 123; // bar/test.js -var r = {}; -f(r, { - bar: () => m +var t = {}; +s(t, { + bar: () => l }); -var m = 123; +var l = 123; // entry.js -console.log(exports, module.exports, t, r); +console.log(exports, module.exports, o, t); ================================================================================ TestMinifyArguments @@ -1698,6 +1714,7 @@ TestReExportDefaultCommonJS ---------- /out.js ---------- // bar.js var require_bar = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => foo }); @@ -1716,6 +1733,7 @@ import_bar.default(); TestReExportDefaultExternalCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __export(exports, { bar: () => import_bar.default, foo: () => import_foo.default @@ -1760,6 +1778,7 @@ export {default as bar} from "./bar"; ================================================================================ TestReExportDefaultNoBundleCommonJS ---------- /out.js ---------- +__markAsModule(exports); __export(exports, { bar: () => import_bar.default, foo: () => import_foo.default @@ -2173,6 +2192,7 @@ var require_cjs = __commonJS((exports) => { // dummy.js var require_dummy = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { dummy: () => dummy }); @@ -2199,6 +2219,7 @@ var require_es6_expr_import_dynamic = __commonJS((exports) => { // es6-export-star.js var require_es6_export_star = __commonJS((exports) => { + __markAsModule(exports); __exportStar(exports, __toModule(require_dummy())); console.log(void 0); }); @@ -2401,6 +2422,7 @@ TestTopLevelReturn ---------- /out.js ---------- // foo.js var require_foo = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo2 }); diff --git a/internal/bundler/snapshots/snapshots_importstar.txt b/internal/bundler/snapshots/snapshots_importstar.txt index 2cf20d75502..044f99ca2ee 100644 --- a/internal/bundler/snapshots/snapshots_importstar.txt +++ b/internal/bundler/snapshots/snapshots_importstar.txt @@ -6,6 +6,7 @@ var require_foo = __commonJS((exports2) => { }); // entry.js +__markAsModule(exports); __export(exports, { ns: () => ns }); @@ -20,6 +21,7 @@ var require_foo = __commonJS((exports2) => { }); // entry.js +__markAsModule(exports); __export(exports, { bar: () => import_foo.bar }); @@ -34,6 +36,7 @@ var require_foo = __commonJS((exports2) => { }); // entry.js +__markAsModule(exports); __export(exports, { y: () => import_foo.x }); @@ -45,6 +48,7 @@ var import_foo = __toModule(require_foo()); TestExportSelfAndImportSelfCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __export(exports, { foo: () => foo }); @@ -56,6 +60,7 @@ TestExportSelfAndRequireSelfCommonJS ---------- /out.js ---------- // entry.js var require_entry = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo }); @@ -68,6 +73,7 @@ module.exports = require_entry(); TestExportSelfAsNamespaceCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __export(exports, { foo: () => foo, ns: () => exports @@ -93,6 +99,7 @@ export { TestExportSelfCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __export(exports, { foo: () => foo }); @@ -142,6 +149,7 @@ var someName = (() => { TestExportStarDefaultExportCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __export(exports, { foo: () => foo }); @@ -248,6 +256,7 @@ var require_foo = __commonJS((exports2) => { }); // entry.js +__markAsModule(exports); __export(exports, { ns: () => ns }); @@ -324,6 +333,7 @@ TestImportStarAndCommonJS ---------- /out.js ---------- // foo.js var require_foo = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo2 }); @@ -744,6 +754,7 @@ export { ================================================================================ TestReExportStarAsCommonJSNoBundle ---------- /out.js ---------- +__markAsModule(exports); __export(exports, { out: () => out }); @@ -761,6 +772,7 @@ export { TestReExportStarAsExternalCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __export(exports, { out: () => out }); @@ -803,6 +815,7 @@ var mod = (() => { ================================================================================ TestReExportStarCommonJSNoBundle ---------- /out.js ---------- +__markAsModule(exports); __exportStar(exports, __toModule(require("foo"))); ================================================================================ @@ -814,6 +827,7 @@ export * from "foo"; TestReExportStarExternalCommonJS ---------- /out.js ---------- // entry.js +__markAsModule(exports); __exportStar(exports, __toModule(require("foo"))); ================================================================================ @@ -828,6 +842,7 @@ TestReExportStarExternalIIFE var mod = (() => { // entry.js var require_entry = __commonJS((exports) => { + __markAsModule(exports); __exportStar(exports, __toModule(require("foo"))); }); return require_entry(); @@ -838,6 +853,7 @@ TestReExportStarIIFENoBundle ---------- /out.js ---------- var mod = (() => { var require_entry = __commonJS((exports) => { + __markAsModule(exports); __exportStar(exports, __toModule(require("foo"))); }); return require_entry(); diff --git a/internal/bundler/snapshots/snapshots_importstar_ts.txt b/internal/bundler/snapshots/snapshots_importstar_ts.txt index 7c02ec7f64f..4e829d0e720 100644 --- a/internal/bundler/snapshots/snapshots_importstar_ts.txt +++ b/internal/bundler/snapshots/snapshots_importstar_ts.txt @@ -2,6 +2,7 @@ TestTSImportStarAndCommonJS ---------- /out.js ---------- // foo.ts var require_foo = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo2 }); diff --git a/internal/bundler/snapshots/snapshots_packagejson.txt b/internal/bundler/snapshots/snapshots_packagejson.txt index 99ce597e245..836d2a29998 100644 --- a/internal/bundler/snapshots/snapshots_packagejson.txt +++ b/internal/bundler/snapshots/snapshots_packagejson.txt @@ -329,6 +329,7 @@ TestPackageJsonDualPackageHazardImportAndRequireForceModuleBeforeMain ---------- /Users/user/project/out.js ---------- // Users/user/project/node_modules/demo-pkg/module.js var require_module = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { default: () => module_default }); diff --git a/internal/bundler/snapshots/snapshots_splitting.txt b/internal/bundler/snapshots/snapshots_splitting.txt index 02b1fb333e3..94a76471f07 100644 --- a/internal/bundler/snapshots/snapshots_splitting.txt +++ b/internal/bundler/snapshots/snapshots_splitting.txt @@ -307,7 +307,7 @@ TestSplittingHybridCJSAndESMIssue617 ---------- /out/a.js ---------- import { require_a -} from "./chunk.UM5LDUBH.js"; +} from "./chunk.3PFM4BVK.js"; export default require_a(); ---------- /out/b.js ---------- @@ -315,7 +315,7 @@ import { __defProp, __markAsModule, require_a -} from "./chunk.UM5LDUBH.js"; +} from "./chunk.3PFM4BVK.js"; // b.js var import_a = __toModule(require_a()); @@ -324,9 +324,10 @@ export { export_foo as foo }; ----------- /out/chunk.UM5LDUBH.js ---------- +---------- /out/chunk.3PFM4BVK.js ---------- // a.js var require_a = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo }); @@ -345,13 +346,13 @@ TestSplittingHybridESMAndCJSIssue617 ---------- /out/a.js ---------- import { require_a -} from "./chunk.YOK3FHV5.js"; +} from "./chunk.EPKGCDSI.js"; export default require_a(); ---------- /out/b.js ---------- import { require_a -} from "./chunk.YOK3FHV5.js"; +} from "./chunk.EPKGCDSI.js"; // b.js var bar = require_a(); @@ -359,9 +360,10 @@ export { bar }; ----------- /out/chunk.YOK3FHV5.js ---------- +---------- /out/chunk.EPKGCDSI.js ---------- // a.js var require_a = __commonJS((exports) => { + __markAsModule(exports); __export(exports, { foo: () => foo }); diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go index 7c2b8f3f02f..c65e2a20e58 100644 --- a/internal/js_ast/js_ast.go +++ b/internal/js_ast/js_ast.go @@ -1689,7 +1689,7 @@ func (ast *AST) UsesCommonJSExports() bool { return ast.UsesExportsRef || ast.UsesModuleRef } -func (ast *AST) HasES6Syntax() bool { +func (ast *AST) HasES6ImportsOrExports() bool { return ast.HasES6Imports || ast.HasES6Exports } diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index c660d400625..a5b5b55bded 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -81,12 +81,10 @@ func code(isES6 bool) string { // Used to implement ES6 exports to CommonJS export var __export = (target, all) => { - __markAsModule(target) for (var name in all) __defProp(target, name, { get: all[name], enumerable: true }) } export var __exportStar = (target, module, desc) => { - __markAsModule(target) if (module && typeof module === 'object' || typeof module === 'function') ` @@ -116,7 +114,7 @@ func code(isES6 bool) string { if (module && module.__esModule) return module return __exportStar( - __defProp(module != null ? __create(__getProtoOf(module)) : {}, 'default', { value: module, enumerable: true }), + __markAsModule(__defProp(module != null ? __create(__getProtoOf(module)) : {}, 'default', { value: module, enumerable: true })), module) } diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 2c2b1329f09..09c0fc981a9 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -288,6 +288,44 @@ } `, }), + + // Test non-bundled double export star + test(['node.ts', 're-export.ts', 'a.ts', 'b.ts', '--format=cjs', '--outdir=.'], { + 'node.ts': ` + import {a, b} from './re-export' + if (a !== 'a' || b !== 'b') throw 'fail' + `, + 're-export.ts': ` + export * from './a' + export * from './b' + `, + 'a.ts': ` + export let a = 'a' + `, + 'b.ts': ` + export let b = 'b' + `, + }), + + // Complex circular non-bundling import case + test(['node.ts', 're-export.ts', 'a.ts', 'b.ts', '--format=cjs', '--outdir=.'], { + 'node.ts': ` + import {a} from './re-export' + let fn = a() + if (fn === a || fn() !== a) throw 'fail' + `, + 're-export.ts': ` + export * from './a' + `, + 'a.ts': ` + import {b} from './b' + export let a = () => b + `, + 'b.ts': ` + import {a} from './re-export' + export let b = () => a + `, + }), ) // Test internal ES6 export