fix #532: use node's "default" semantics for cjs-in-esm

CHANGELOG.md

@@ -2,6 +2,18 @@
 
 ## Unreleased
 
+* Align with node's `default` import behavior for CommonJS ([#532](https://github.com/evanw/esbuild/issues/532))
+
+    _Note: This could be considered a breaking change or a bug fix depending on your point of view._
+
+    Importing a CommonJS file into an ESM file does not behave the same everywhere. Historically people compiled their ESM code into CommonJS using Babel before ESM was supported natively. More recently, node has made it possible to use ESM syntax natively but to still import CommonJS files into ESM. These behave differently in many ways but one of the most unfortunate differences is how the `default` export is handled.
+
+    When you import a normal CommonJS file, both Babel and node agree that the value of `module.exports` should be stored in the ESM import named `default`. However, if the CommonJS file used to be an ESM file but was compiled into a CommonJS file, Babel will set the ESM import named `default` to the value of the original ESM export named `default` while node will continue to set the ESM import named `default` to the value of `module.exports`. Babel detects if a CommonJS file used to be an ESM file by the presence of the `exports.__esModule = true` marker.
+
+    This is unfortunate because it means there is no general way to make code work with both ecosystems. With Babel you can access the original `default` export with `require().default` but with node you need to use `require().default.default` to access the original `default` export. Previously esbuild followed Babel's approach but starting with this release, esbuild will now follow node's approach. Now that node has native ESM support, people will be writing more and more native ESM code and will expect bundlers to follow node semantics. Using ESM via Babel is now the legacy code path that will become less and less relevant over time.
+
+    If you are authoring a library using ESM but shipping it as CommonJS, the best way to avoid this mess is to just never use `default` exports in ESM. Only use named exports with names other than `default`.
+
 * Fix bug when ESM file has empty exports and is converted to CommonJS ([#910](https://github.com/evanw/esbuild/issues/910))
 
     A file containing the contents `export {}` is still considered to be an ESM file even though it has no exports. However, if a file containing this edge case is converted to CommonJS internally during bundling (e.g. when it is the target of `require()`), esbuild failed to mark the `exports` symbol from the CommonJS wrapping closure as used even though it is actually needed. This resulted in an output file that crashed when run. The `exports` symbol is now considered used in this case, so the bug has been fixed.

internal/bundler/linker.go

@@ -167,6 +167,42 @@ type fileMeta struct {
 	// this module must be added as getters to the CommonJS "exports" object.
 	cjsStyleExports bool
 
+	// WARNING: This is an interop mess. Different tools do different things and
+	// there really isn't a good solution that will always work. More information:
+	// https://github.com/evanw/esbuild/issues/532.
+	//
+	// The value of the "default" export differs between node's ESM
+	// implementation and Babel's cross-compiled ESM-to-CommonJS
+	// implementation. The default export will always be "module.exports"
+	// in node but will be "module.exports.default" with Babel if the
+	// property "module.exports.__esModule" is true (indicating a
+	// cross-compiled ESM file):
+	//
+	//   // esm-file.mjs
+	//   import defaultValue from "./import.cjs"
+	//   console.log(defaultValue)
+	//
+	//   // import.cjs (cross-compiled from "import.esm")
+	//   Object.defineProperty(exports, '__esModule', { value: true })
+	//   exports.default = 'default'
+	//
+	//   // import.mjs (original source code for "import.cjs")
+	//   export default 'default'
+	//
+	// Code that respects the "__esModule" flag will print "default" but node
+	// will print "{ default: 'default' }". There is no way to work with both.
+	// Damned if you do, damned if you don't. It would have been ideal if node
+	// behaved consistently with the rest of the ecosystem, but they decided to
+	// do their own thing instead. Arguably no approach is "more correct" than
+	// the other one.
+	//
+	// We need to behave like Babel when we cross-compile ESM to CommonJS but
+	// we need to behave like node for compatibility with existing libraries
+	// on npm. So we deliberately skip calling "__toModule" only for ESM files
+	// that we ourselves have converted to CommonJS during the build so that we
+	// at least don't break ourselves.
+	skipCallingToModule bool
+
 	// If true, the "__export(exports, { ... })" call will be force-included even
 	// if there are no parts that reference "exports". Otherwise this call will
 	// be removed due to the tree shaking pass. This is used when for entry point
@@ -436,6 +472,7 @@ func newLinkerContext(
 				resolvedExports:          resolvedExports,
 				isProbablyTypeScriptType: make(map[js_ast.Ref]bool),
 				importsToBind:            make(map[js_ast.Ref]importToBind),
+				skipCallingToModule:      repr.ast.HasES6Exports && !repr.ast.HasCommonJSFeatures(),
 			}
 
 		case *reprCSS:
@@ -2601,7 +2638,7 @@ func (c *linkerContext) includePart(sourceIndex uint32, partIndex uint32, entryP
 
 		// This is an ES6 import of a CommonJS module, so it needs the
 		// "__toModule" wrapper as long as it's not a bare "require()"
-		if record.Kind != ast.ImportRequire {
+		if record.Kind != ast.ImportRequire && !otherRepr.meta.skipCallingToModule {
 			record.WrapWithToModule = true
 			toModuleUses++
 		}

internal/bundler/snapshots/snapshots_dce.txt

@@ -152,7 +152,7 @@ var require_index_main = __commonJS((exports) => {
 });
 
 // Users/user/project/src/entry.js
-var import_demo_pkg = __toModule(require_index_main());
+var import_demo_pkg = require_index_main();
 
 // Users/user/project/src/require-demo-pkg.js
 require_index_main();

internal/bundler/snapshots/snapshots_default.txt

@@ -2414,20 +2414,20 @@ var require_es6_import_assign = __commonJS((exports) => {
 
 // es6-import-dynamic.js
 var require_es6_import_dynamic = __commonJS((exports) => {
-  Promise.resolve().then(() => __toModule(require_dummy()));
+  Promise.resolve().then(() => require_dummy());
   console.log(exports);
 });
 
 // es6-expr-import-dynamic.js
 var require_es6_expr_import_dynamic = __commonJS((exports) => {
-  Promise.resolve().then(() => __toModule(require_dummy()));
+  Promise.resolve().then(() => require_dummy());
   console.log(exports);
 });
 
 // es6-export-star.js
 var require_es6_export_star = __commonJS((exports) => {
   __markAsModule(exports);
-  __exportStar(exports, __toModule(require_dummy()));
+  __exportStar(exports, require_dummy());
   console.log(void 0);
 });
 
@@ -2526,7 +2526,7 @@ var require_es6_ns_export_abstract_class = __commonJS((exports) => {
 var import_cjs = __toModule(require_cjs());
 
 // es6-import-stmt.js
-var import_dummy = __toModule(require_dummy());
+var import_dummy = require_dummy();
 console.log(void 0);
 
 // entry.js
@@ -2582,14 +2582,14 @@ console.log(void 0);
 console.log(void 0);
 
 // es6-export-clause-from.js
-var import_dummy2 = __toModule(require_dummy());
+var import_dummy2 = require_dummy();
 console.log(void 0);
 
 // entry.js
-var import_es6_export_star = __toModule(require_es6_export_star());
+var import_es6_export_star = require_es6_export_star();
 
 // es6-export-star-as.js
-var ns = __toModule(require_dummy());
+var ns = require_dummy();
 console.log(void 0);
 
 // entry.js

internal/bundler/snapshots/snapshots_importstar.txt

@@ -341,7 +341,7 @@ var require_foo = __commonJS((exports) => {
 });
 
 // entry.js
-var ns = __toModule(require_foo());
+var ns = require_foo();
 var ns2 = require_foo();
 console.log(ns.foo, ns2.foo);
 

internal/bundler/snapshots/snapshots_importstar_ts.txt

@@ -10,7 +10,7 @@ var require_foo = __commonJS((exports) => {
 });
 
 // entry.js
-var ns = __toModule(require_foo());
+var ns = require_foo();
 var ns2 = require_foo();
 console.log(ns.foo, ns2.foo);
 

internal/bundler/snapshots/snapshots_packagejson.txt

@@ -340,7 +340,7 @@ var require_module = __commonJS((exports) => {
 console.log(require_module());
 
 // Users/user/project/src/test-module.js
-var import_demo_pkg = __toModule(require_module());
+var import_demo_pkg = require_module();
 console.log(import_demo_pkg.default);
 
 ================================================================================

internal/js_parser/js_parser.go

@@ -11249,9 +11249,10 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 				if symbol := p.symbols[id.Ref.InnerIndex]; symbol.OriginalName == "eval" {
 					e.IsDirectEval = true
 
-					// Pessimistically assume that a direct call to "eval" means that code could
-					// potentially access "module" or "exports".
-					if p.options.mode == config.ModeBundle {
+					// Pessimistically assume that if this looks like a CommonJS module
+					// (no "import" or "export" keywords), a direct call to "eval" means
+					// that code could potentially access "module" or "exports".
+					if p.options.mode == config.ModeBundle && p.es6ImportKeyword.Len == 0 && p.es6ExportKeyword.Len == 0 {
 						p.recordUsage(p.moduleRef)
 						p.recordUsage(p.exportsRef)
 					}

internal/runtime/runtime.go

@@ -111,8 +111,6 @@ func code(isES6 bool) string {
 
 		// Converts the module from CommonJS to ES6 if necessary
 		export var __toModule = module => {
-			if (module && module.__esModule)
-				return module
 			return __exportStar(
 				__markAsModule(__defProp(module != null ? __create(__getProtoOf(module)) : {}, 'default', { value: module, enumerable: true })),
 				module)

scripts/end-to-end-tests.js

@@ -349,8 +349,8 @@
       `,
     }),
 
-    // Test non-bundled double export star
-    test(['node.ts', 're-export.ts', 'a.ts', 'b.ts', '--format=cjs', '--outdir=.'], {
+    // Test bundled and non-bundled double export star
+    test(['node.ts', '--bundle', '--format=cjs', '--outdir=.'], {
       'node.ts': `
         import {a, b} from './re-export'
         if (a !== 'a' || b !== 'b') throw 'fail'
@@ -366,13 +366,97 @@
         export let b = 'b'
       `,
     }),
+    test(['node.ts', '--bundle', '--format=cjs', '--outdir=.'], {
+      'node.ts': `
+        import {a, b} from './re-export'
+        if (a !== 'a' || b !== 'b') throw 'fail'
 
-    // Complex circular non-bundling import case
+        // Try forcing all of these modules to be CommonJS wrappers
+        require('./node')
+        require('./re-export')
+        require('./a')
+        require('./b')
+      `,
+      're-export.ts': `
+        export * from './a'
+        export * from './b'
+      `,
+      'a.ts': `
+        export let a = 'a'
+      `,
+      'b.ts': `
+        export let b = 'b'
+      `,
+    }),
     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 bundled and non-bundled import case (https://github.com/evanw/esbuild/issues/758)
+    test(['node.ts', '--bundle', '--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(['node.ts', '--bundle', '--format=cjs', '--outdir=.'], {
       'node.ts': `
         import {a} from './re-export'
         let fn = a()
         if (fn === a || fn() !== a) throw 'fail'
+
+        // Try forcing all of these modules to be CommonJS wrappers
+        require('./node')
+        require('./re-export')
+        require('./a')
+        require('./b')
+      `,
+      '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(['node.ts', 're-export.ts', 'a.ts', 'b.ts', '--format=cjs', '--outdir=.'], {
+      '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'
       `,
       're-export.ts': `
         export * from './a'