diff --git a/CHANGELOG.md b/CHANGELOG.md index c56c67acfb0..ec62fb03ee1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Previously esbuild always created the forwarding `default` import in Babel mode, even if `module.exports` had no property called `default`. This was problematic because the getter named `default` still showed up as a property on the imported namespace object, and could potentially interfere with code that iterated over the properties of the imported namespace object. With this release the getter named `default` will now only be added in Babel mode if the `default` property exists at the time of the import. +* Fix a circular import edge case regarding ESM-to-CommonJS conversion ([#1894](https://github.com/evanw/esbuild/issues/1894), [#2059](https://github.com/evanw/esbuild/pull/2059)) + + This fixes a regression that was introduced in version 0.14.5 of esbuild. Ever since that version, esbuild now creates two separate export objects when you convert an ES module file into a CommonJS module: one for ES modules and one for CommonJS modules. The one for CommonJS modules is written to `module.exports` and exported from the file, and the one for ES modules is internal and can be accessed by bundling code that imports the entry point (for example, the entry point might import itself to be able to inspect its own exports). + + The reason for these two separate export objects is that CommonJS modules are supposed to see a special export called `__esModule` which indicates that the module used to be an ES module, while ES modules are not supposed to see any automatically-added export named `__esModule`. This matters for real-world code both because people sometimes iterate over the properties of ES module export namespace objects and because some people write ES module code containing their own exports named `__esModule` that they expect other ES module code to be able to read. + + However, this change to split exports into two separate objects broke ES module re-exports in the edge case where the imported module is involved in an import cycle. This happened because the CommonJS `module.exports` object was no longer mutated as exports were added. Instead it was being initialized at the end of the generated file after the import statements to other modules (which are converted into `require()` calls). This release changes `module.exports` initialization to happen earlier in the file and then double-writes further exports to both the ES module and CommonJS module export objects. + + This fix was contributed by [@indutny](https://github.com/indutny). + ## 0.14.26 * Fix a tree shaking regression regarding `var` declarations ([#2080](https://github.com/evanw/esbuild/issues/2080), [#2085](https://github.com/evanw/esbuild/pull/2085), [#2098](https://github.com/evanw/esbuild/issues/2098), [#2099](https://github.com/evanw/esbuild/issues/2099)) diff --git a/internal/bundler/bundler_importstar_test.go b/internal/bundler/bundler_importstar_test.go index 1338407e72f..291c861c9f7 100644 --- a/internal/bundler/bundler_importstar_test.go +++ b/internal/bundler/bundler_importstar_test.go @@ -1805,3 +1805,33 @@ entry-nope.js: WARNING: Import "nope" will always be undefined because the file `, }) } + +// Failure case due to a bug in https://github.com/evanw/esbuild/pull/2059 +func TestReExportStarEntryPointAndInnerFile(t *testing.T) { + importstar_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + export * from 'a' + import * as inner from './inner.js' + export { inner } + `, + "/inner.js": ` + export * from 'b' + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + OutputFormat: config.FormatCommonJS, + ExternalSettings: config.ExternalSettings{ + PreResolve: config.ExternalMatchers{ + Exact: map[string]bool{ + "a": true, + "b": true, + }, + }, + }, + }, + }) +} diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index ada9c255761..33175022033 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -2024,6 +2024,31 @@ func (c *linkerContext) createExportsForFile(sourceIndex uint32) { repr.AST.UsesExportsRef = true } + // Decorate "module.exports" with the "__esModule" flag to indicate that + // we used to be an ES module. This is done by wrapping the exports object + // instead of by mutating the exports object because other modules in the + // bundle (including the entry point module) may do "import * as" to get + // access to the exports object and should NOT see the "__esModule" flag. + if repr.Meta.ForceIncludeExportsForEntryPoint && + c.options.OutputFormat == config.FormatCommonJS { + + runtimeRepr := c.graph.Files[runtime.SourceIndex].InputFile.Repr.(*graph.JSRepr) + toCommonJSRef := runtimeRepr.AST.NamedExports["__toCommonJS"].Ref + + // "module.exports = __toCommonJS(exports);" + nsExportStmts = append(nsExportStmts, js_ast.AssignStmt( + js_ast.Expr{Data: &js_ast.EDot{ + Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: c.unboundModuleRef}}, + Name: "exports", + }}, + + js_ast.Expr{Data: &js_ast.ECall{ + Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: toCommonJSRef}}, + Args: []js_ast.Expr{{Data: &js_ast.EIdentifier{Ref: repr.AST.ExportsRef}}}, + }}, + )) + } + // No need to generate a part if it'll be empty if len(nsExportStmts) > 0 { // Initialize the part that was allocated for us earlier. The information @@ -3492,6 +3517,21 @@ func (c *linkerContext) convertStmtsForChunk(sourceIndex uint32, stmtList *stmtL repr := file.InputFile.Repr.(*graph.JSRepr) shouldExtractESMStmtsForWrap := repr.Meta.Wrap != graph.WrapNone + // If this file is a CommonJS entry point, double-write re-exports to the + // external CommonJS "module.exports" object in addition to our internal ESM + // export namespace object. The difference between these two objects is that + // our internal one must not have the "__esModule" marker while the external + // one must have the "__esModule" marker. This is done because an ES module + // importing itself should not see the "__esModule" marker but a CommonJS module + // importing us should see the "__esModule" marker. + var moduleExportsForReExportOrNil js_ast.Expr + if c.options.OutputFormat == config.FormatCommonJS && file.IsEntryPoint() { + moduleExportsForReExportOrNil = js_ast.Expr{Data: &js_ast.EDot{ + Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: c.unboundModuleRef}}, + Name: "exports", + }} + } + for _, stmt := range partStmts { switch s := stmt.Data.(type) { case *js_ast.SImport: @@ -3547,16 +3587,20 @@ func (c *linkerContext) convertStmtsForChunk(sourceIndex uint32, stmtList *stmtL ImportRecordIndex: s.ImportRecordIndex, } - // Prefix this module with "__reExport(exports, ns)" + // Prefix this module with "__reExport(exports, ns, module.exports)" exportStarRef := c.graph.Files[runtime.SourceIndex].InputFile.Repr.(*graph.JSRepr).AST.ModuleScope.Members["__reExport"].Ref + args := []js_ast.Expr{ + {Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: repr.AST.ExportsRef}}, + {Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: s.NamespaceRef}}, + } + if moduleExportsForReExportOrNil.Data != nil { + args = append(args, moduleExportsForReExportOrNil) + } stmtList.insideWrapperPrefix = append(stmtList.insideWrapperPrefix, js_ast.Stmt{ Loc: stmt.Loc, Data: &js_ast.SExpr{Value: js_ast.Expr{Loc: stmt.Loc, Data: &js_ast.ECall{ Target: js_ast.Expr{Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: exportStarRef}}, - Args: []js_ast.Expr{ - {Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: repr.AST.ExportsRef}}, - {Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: s.NamespaceRef}}, - }, + Args: args, }}}, }) @@ -3579,25 +3623,29 @@ func (c *linkerContext) convertStmtsForChunk(sourceIndex uint32, stmtList *stmtL var target js_ast.E if record.SourceIndex.IsValid() { if otherRepr := c.graph.Files[record.SourceIndex.GetIndex()].InputFile.Repr.(*graph.JSRepr); otherRepr.AST.ExportsKind == js_ast.ExportsESMWithDynamicFallback { - // Prefix this module with "__reExport(exports, otherExports)" + // Prefix this module with "__reExport(exports, otherExports, module.exports)" target = &js_ast.EIdentifier{Ref: otherRepr.AST.ExportsRef} } } if target == nil { - // Prefix this module with "__reExport(exports, require(path))" + // Prefix this module with "__reExport(exports, require(path), module.exports)" target = &js_ast.ERequireString{ ImportRecordIndex: s.ImportRecordIndex, } } exportStarRef := c.graph.Files[runtime.SourceIndex].InputFile.Repr.(*graph.JSRepr).AST.ModuleScope.Members["__reExport"].Ref + args := []js_ast.Expr{ + {Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: repr.AST.ExportsRef}}, + {Loc: record.Range.Loc, Data: target}, + } + if moduleExportsForReExportOrNil.Data != nil { + args = append(args, moduleExportsForReExportOrNil) + } stmtList.insideWrapperPrefix = append(stmtList.insideWrapperPrefix, js_ast.Stmt{ Loc: stmt.Loc, Data: &js_ast.SExpr{Value: js_ast.Expr{Loc: stmt.Loc, Data: &js_ast.ECall{ Target: js_ast.Expr{Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: exportStarRef}}, - Args: []js_ast.Expr{ - {Loc: stmt.Loc, Data: &js_ast.EIdentifier{Ref: repr.AST.ExportsRef}}, - {Loc: record.Range.Loc, Data: target}, - }, + Args: args, }}}, }) } @@ -4097,25 +4145,6 @@ func (c *linkerContext) generateEntryPointTailJS( Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.AST.WrapperRef}}, }}}}) } - - // Decorate "module.exports" with the "__esModule" flag to indicate that - // we used to be an ES module. This is done by wrapping the exports object - // instead of by mutating the exports object because other modules in the - // bundle (including the entry point module) may do "import * as" to get - // access to the exports object and should NOT see the "__esModule" flag. - if repr.Meta.ForceIncludeExportsForEntryPoint { - // "module.exports = __toCommonJS(exports);" - stmts = append(stmts, js_ast.AssignStmt( - js_ast.Expr{Data: &js_ast.EDot{ - Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: c.unboundModuleRef}}, - Name: "exports", - }}, - js_ast.Expr{Data: &js_ast.ECall{ - Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: toCommonJSRef}}, - Args: []js_ast.Expr{{Data: &js_ast.EIdentifier{Ref: repr.AST.ExportsRef}}}, - }}, - )) - } } // If we are generating CommonJS for node, encode the known export names in diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index 42da6186108..9b265dadb08 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -857,8 +857,8 @@ TestExportWildcardFSNodeCommonJS ---------- /out.js ---------- // entry.js var entry_exports = {}; -__reExport(entry_exports, require("fs")); module.exports = __toCommonJS(entry_exports); +__reExport(entry_exports, require("fs"), module.exports); ================================================================================ TestExportWildcardFSNodeES6 @@ -2440,11 +2440,11 @@ __export(entry_exports, { bar: () => import_bar.default, foo: () => import_foo.default }); +module.exports = __toCommonJS(entry_exports); var import_foo = __toESM(require("foo")); // bar.js var import_bar = __toESM(require("bar")); -module.exports = __toCommonJS(entry_exports); ================================================================================ TestReExportDefaultExternalES6 @@ -2486,9 +2486,9 @@ __export(entry_exports, { bar: () => import_bar.default, foo: () => import_foo.default }); +module.exports = __toCommonJS(entry_exports); var import_foo = __toESM(require("./foo")); var import_bar = __toESM(require("./bar")); -module.exports = __toCommonJS(entry_exports); ================================================================================ TestReExportDefaultNoBundleES6 diff --git a/internal/bundler/snapshots/snapshots_importstar.txt b/internal/bundler/snapshots/snapshots_importstar.txt index 23be9e99c8a..c3bb49ae396 100644 --- a/internal/bundler/snapshots/snapshots_importstar.txt +++ b/internal/bundler/snapshots/snapshots_importstar.txt @@ -12,8 +12,8 @@ var entry_exports = {}; __export(entry_exports, { ns: () => ns }); -var ns = __toESM(require_foo()); module.exports = __toCommonJS(entry_exports); +var ns = __toESM(require_foo()); ================================================================================ TestExportOtherCommonJS @@ -30,8 +30,8 @@ var entry_exports = {}; __export(entry_exports, { bar: () => import_foo.bar }); -var import_foo = __toESM(require_foo()); module.exports = __toCommonJS(entry_exports); +var import_foo = __toESM(require_foo()); ================================================================================ TestExportOtherNestedCommonJS @@ -48,10 +48,10 @@ var entry_exports = {}; __export(entry_exports, { y: () => import_foo.x }); +module.exports = __toCommonJS(entry_exports); // bar.js var import_foo = __toESM(require_foo()); -module.exports = __toCommonJS(entry_exports); ================================================================================ TestExportSelfAndImportSelfCommonJS @@ -61,9 +61,9 @@ var entry_exports = {}; __export(entry_exports, { foo: () => foo }); +module.exports = __toCommonJS(entry_exports); var foo = 123; console.log(entry_exports); -module.exports = __toCommonJS(entry_exports); ================================================================================ TestExportSelfAndRequireSelfCommonJS @@ -73,6 +73,7 @@ var entry_exports = {}; __export(entry_exports, { foo: () => foo }); +module.exports = __toCommonJS(entry_exports); var foo; var init_entry = __esm({ "entry.js"() { @@ -81,7 +82,6 @@ var init_entry = __esm({ } }); init_entry(); -module.exports = __toCommonJS(entry_exports); ================================================================================ TestExportSelfAsNamespaceCommonJS @@ -92,8 +92,8 @@ __export(entry_exports, { foo: () => foo, ns: () => entry_exports }); -var foo = 123; module.exports = __toCommonJS(entry_exports); +var foo = 123; ================================================================================ TestExportSelfAsNamespaceES6 @@ -118,8 +118,8 @@ var entry_exports = {}; __export(entry_exports, { foo: () => foo }); -var foo = 123; module.exports = __toCommonJS(entry_exports); +var foo = 123; ================================================================================ TestExportSelfCommonJSMinified @@ -169,10 +169,10 @@ var entry_exports = {}; __export(entry_exports, { foo: () => foo }); +module.exports = __toCommonJS(entry_exports); // foo.js var foo = "foo"; -module.exports = __toCommonJS(entry_exports); ================================================================================ TestImportDefaultNamespaceComboIssue446 @@ -275,8 +275,8 @@ var entry_exports = {}; __export(entry_exports, { ns: () => ns }); -var ns = __toESM(require_foo()); module.exports = __toCommonJS(entry_exports); +var ns = __toESM(require_foo()); ================================================================================ TestImportExportSelfAsNamespaceES6 @@ -869,8 +869,8 @@ var entry_exports = {}; __export(entry_exports, { out: () => out }); -var out = __toESM(require("foo")); module.exports = __toCommonJS(entry_exports); +var out = __toESM(require("foo")); ================================================================================ TestReExportStarAsES6NoBundle @@ -888,8 +888,8 @@ var entry_exports = {}; __export(entry_exports, { out: () => out }); -var out = __toESM(require("foo")); module.exports = __toCommonJS(entry_exports); +var out = __toESM(require("foo")); ================================================================================ TestReExportStarAsExternalES6 @@ -929,21 +929,36 @@ var mod = (() => { TestReExportStarCommonJSNoBundle ---------- /out.js ---------- var entry_exports = {}; -__reExport(entry_exports, require("foo")); module.exports = __toCommonJS(entry_exports); +__reExport(entry_exports, require("foo"), module.exports); ================================================================================ TestReExportStarES6NoBundle ---------- /out.js ---------- export * from "foo"; +================================================================================ +TestReExportStarEntryPointAndInnerFile +---------- /out/entry.js ---------- +// entry.js +var entry_exports = {}; +__export(entry_exports, { + inner: () => inner_exports +}); +module.exports = __toCommonJS(entry_exports); +__reExport(entry_exports, require("a"), module.exports); + +// inner.js +var inner_exports = {}; +__reExport(inner_exports, require("b")); + ================================================================================ TestReExportStarExternalCommonJS ---------- /out.js ---------- // entry.js var entry_exports = {}; -__reExport(entry_exports, require("foo")); module.exports = __toCommonJS(entry_exports); +__reExport(entry_exports, require("foo"), module.exports); ================================================================================ TestReExportStarExternalES6 diff --git a/internal/bundler/snapshots/snapshots_loader.txt b/internal/bundler/snapshots/snapshots_loader.txt index 73960387028..62527b64c52 100644 --- a/internal/bundler/snapshots/snapshots_loader.txt +++ b/internal/bundler/snapshots/snapshots_loader.txt @@ -965,9 +965,10 @@ export default { TestToESMWrapperOmission ---------- /out/entry.js ---------- var entry_exports = {}; +module.exports = __toCommonJS(entry_exports); var import_a_nowrap = require("a_nowrap"); var import_b_nowrap = require("b_nowrap"); -__reExport(entry_exports, require("c_nowrap")); +__reExport(entry_exports, require("c_nowrap"), module.exports); var d = __toESM(require("d_WRAP")); var import_e_WRAP = __toESM(require("e_WRAP")); var import_f_WRAP = __toESM(require("f_WRAP")); @@ -984,7 +985,6 @@ x = h; i.x(); j.x``; x = Promise.resolve().then(() => __toESM(require("k_WRAP"))); -module.exports = __toCommonJS(entry_exports); ================================================================================ TestWarnCommonJSExportsInESMBundle @@ -994,10 +994,10 @@ var cjs_in_esm_exports = {}; __export(cjs_in_esm_exports, { foo: () => foo }); +module.exports = __toCommonJS(cjs_in_esm_exports); var foo = 1; exports.foo = 2; module.exports = 3; -module.exports = __toCommonJS(cjs_in_esm_exports); ---------- /out/import-in-cjs.js ---------- // import-in-cjs.js @@ -1016,19 +1016,19 @@ var cjs_in_esm_exports = {}; __export(cjs_in_esm_exports, { foo: () => foo }); +module.exports = __toCommonJS(cjs_in_esm_exports); let foo = 1; exports.foo = 2; module.exports = 3; -module.exports = __toCommonJS(cjs_in_esm_exports); ---------- /out/cjs-in-esm2.js ---------- var cjs_in_esm2_exports = {}; __export(cjs_in_esm2_exports, { foo: () => foo }); +module.exports = __toCommonJS(cjs_in_esm2_exports); let foo = 1; module.exports.bar = 3; -module.exports = __toCommonJS(cjs_in_esm2_exports); ---------- /out/import-in-cjs.js ---------- var import_bar = require("bar"); diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 801972e54bb..92f162cf854 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -208,7 +208,15 @@ func code(isES6 bool) string { return to } - export var __reExport = (target, mod) => __copyProps(target, mod, 'default') + // This is used to implement "export * from" statements. It copies properties + // from the imported module to the current module's ESM export object. If the + // current module is an entry point and the target format is CommonJS, we + // also copy the properties to "module.exports" in addition to our module's + // internal ESM export object. + export var __reExport = (target, mod, secondTarget) => ( + __copyProps(target, mod, 'default'), + secondTarget && __copyProps(secondTarget, mod, 'default') + ) // Converts the module from CommonJS to ESM. When in node mode (i.e. in an // ".mjs" file, package.json has "type: module", or the "__esModule" export diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 7f5931eafaa..b3ac31e674f 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -1100,11 +1100,7 @@ 'node.ts': ` import {a} from './re-export' let fn = a() - - // Note: The "void 0" is different here. This case broke when fixing - // something else ("default" export semantics in node). This test still - // exists to document this broken behavior. - if (fn === a || fn() !== void 0) throw 'fail' + if (fn === a || fn() !== a) throw 'fail' `, 're-export.ts': ` export * from './a' @@ -1119,6 +1115,38 @@ `, }), + // Failure case due to a bug in https://github.com/evanw/esbuild/pull/2059 + test(['in.ts', '--bundle', '--format=cjs', '--outfile=out.js', '--external:*.cjs'], { + 'in.ts': ` + export * from './a.cjs' + import * as inner from './inner.js' + export { inner } + `, + 'inner.ts': `export * from './b.cjs'`, + 'a.cjs': `exports.a = 'a'`, + 'b.cjs': `exports.b = 'b'`, + 'node.js': ` + const out = require('./out.js') + if (out.a !== 'a' || out.inner === void 0 || out.inner.b !== 'b' || out.b !== void 0) throw 'fail' + `, + }), + + // Validate internal and external export correctness regarding "__esModule". + // An ES module importing itself should not see "__esModule". But a CommonJS + // module importing an ES module should see "__esModule". + test(['in.ts', '--bundle', '--format=cjs', '--outfile=out.js', '--external:*.cjs'], { + 'in.ts': ` + export * from './a.cjs' + import * as us from './in.js' + if (us.a !== 'a' || us.__esModule !== void 0) throw 'fail' + `, + 'a.cjs': `exports.a = 'a'`, + 'node.js': ` + const out = require('./out.js') + if (out.a !== 'a' || out.__esModule !== true) throw 'fail' + `, + }), + // Use "eval" to access CommonJS variables test(['--bundle', 'in.js', '--outfile=node.js'], { 'in.js': `if (require('./eval').foo !== 123) throw 'fail'`,