Skip to content

Commit

Permalink
Move module.exports assigment to the top (#2059)
Browse files Browse the repository at this point in the history
  • Loading branch information
@indutny
indutny committed Mar 14, 2022
1 parent 73a539d commit a599d45
Show file tree
Hide file tree
Showing 8 changed files with 177 additions and 57 deletions.
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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))
Expand Down
30 changes: 30 additions & 0 deletions internal/bundler/bundler_importstar_test.go
View file
Expand Up @@ -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,
},
},
},
},
})
}
89 changes: 59 additions & 30 deletions internal/bundler/linker.go
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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,
}}},
})

Expand All @@ -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,
}}},
})
}
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions internal/bundler/snapshots/snapshots_default.txt
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
41 changes: 28 additions & 13 deletions internal/bundler/snapshots/snapshots_importstar.txt
View file
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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"() {
Expand All @@ -81,7 +82,6 @@ var init_entry = __esm({
}
});
init_entry();
module.exports = __toCommonJS(entry_exports);

================================================================================
TestExportSelfAsNamespaceCommonJS
Expand All @@ -92,8 +92,8 @@ __export(entry_exports, {
foo: () => foo,
ns: () => entry_exports
});
var foo = 123;
module.exports = __toCommonJS(entry_exports);
var foo = 123;

================================================================================
TestExportSelfAsNamespaceES6
Expand All @@ -118,8 +118,8 @@ var entry_exports = {};
__export(entry_exports, {
foo: () => foo
});
var foo = 123;
module.exports = __toCommonJS(entry_exports);
var foo = 123;

================================================================================
TestExportSelfCommonJSMinified
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit a599d45

Please sign in to comment.