Move module.exports assigment to the top (#2059)

Author: indutny

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))

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,
+					},
+				},
+			},
+		},
+	})
+}

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

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

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