warn about "module.exports.x = y" in ESM (#1907)

CHANGELOG.md

@@ -1,5 +1,25 @@
 # Changelog
 
+## Unreleased
+
+* Warn about `module.exports.foo = ...` in ESM ([#1907](https://github.com/evanw/esbuild/issues/1907))
+
+    The `module` variable is treated as a global variable reference instead of as a CommonJS module reference in ESM code, which can cause problems for people that try to use both CommonJS and ESM exports in the same file. There has been a warning about this since version 0.14.9. However, the warning only covered cases like `exports.foo = bar` and `module.exports = bar` but not `module.exports.foo = bar`. This last case is now handled;
+
+    ```
+    ▲ [WARNING] The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected
+
+        example.ts:2:0:
+          2 │ module.exports.b = 1
+            ╵ ~~~~~~
+
+      This file is considered to be an ECMAScript module because of the "export" keyword here:
+
+        example.ts:1:0:
+          1 │ export let a = 1
+            ╵ ~~~~~~
+    ```
+
 ## 0.14.10
 
 * Enable tree shaking of classes with lowered static fields ([#175](https://github.com/evanw/esbuild/issues/175))

internal/bundler/bundler_default_test.go

@@ -5218,17 +5218,23 @@ func TestWarnCommonJSExportsInESMConvert(t *testing.T) {
 				exports.foo = 2
 				module.exports = 3
 			`,
+			"/cjs-in-esm2.js": `
+				export let foo = 1
+				module.exports.bar = 3
+			`,
 			"/import-in-cjs.js": `
 				import { foo } from 'bar'
 				exports.foo = foo
 				module.exports = foo
+				module.exports.bar = foo
 			`,
 			"/no-warnings-here.js": `
 				console.log(module, exports)
 			`,
 		},
 		entryPaths: []string{
 			"/cjs-in-esm.js",
+			"/cjs-in-esm2.js",
 			"/import-in-cjs.js",
 			"/no-warnings-here.js",
 		},
@@ -5241,6 +5247,8 @@ func TestWarnCommonJSExportsInESMConvert(t *testing.T) {
 cjs-in-esm.js: NOTE: This file is considered to be an ECMAScript module because of the "export" keyword here:
 cjs-in-esm.js: WARNING: The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected
 cjs-in-esm.js: NOTE: This file is considered to be an ECMAScript module because of the "export" keyword here:
+cjs-in-esm2.js: WARNING: The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected
+cjs-in-esm2.js: NOTE: This file is considered to be an ECMAScript module because of the "export" keyword here:
 `,
 	})
 }

internal/bundler/snapshots/snapshots_loader.txt

@@ -576,10 +576,20 @@ 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
+});
+let foo = 1;
+module.exports.bar = 3;
+module.exports = __toCommonJS(cjs_in_esm2_exports);
+
 ---------- /out/import-in-cjs.js ----------
 var import_bar = require("bar");
 exports.foo = import_bar.foo;
 module.exports = import_bar.foo;
+module.exports.bar = import_bar.foo;
 
 ---------- /out/no-warnings-here.js ----------
 console.log(module, exports);

internal/js_parser/js_parser.go

@@ -11914,21 +11914,43 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 				// to check for a CommonJS environment, and we shouldn't warn on that.
 				if p.options.mode != config.ModePassThrough && p.isFileConsideredToHaveESMExports && !p.isControlFlowDead {
 					if dot, ok := e.Left.Data.(*js_ast.EDot); ok {
-						if id, ok := dot.Target.Data.(*js_ast.EIdentifier); ok {
-							if symbol := &p.symbols[id.Ref.InnerIndex]; symbol.Kind == js_ast.SymbolUnbound &&
+						var name string
+						var loc logger.Loc
+
+						switch target := dot.Target.Data.(type) {
+						case *js_ast.EIdentifier:
+							if symbol := &p.symbols[target.Ref.InnerIndex]; symbol.Kind == js_ast.SymbolUnbound &&
 								((symbol.OriginalName == "module" && dot.Name == "exports") || symbol.OriginalName == "exports") &&
 								!symbol.Flags.Has(js_ast.DidWarnAboutCommonJSInESM) {
 								// "module.exports = ..."
 								// "exports.something = ..."
-								kind := logger.Warning
-								if p.suppressWarningsAboutWeirdCode {
-									kind = logger.Debug
-								}
-								p.log.AddWithNotes(kind, &p.tracker, js_lexer.RangeOfIdentifier(p.source, dot.Target.Loc),
-									fmt.Sprintf("The CommonJS %q variable is treated as a global variable in an ECMAScript module and may not work as expected", symbol.OriginalName),
-									p.whyESModule())
+								name = symbol.OriginalName
+								loc = dot.Target.Loc
 								symbol.Flags |= js_ast.DidWarnAboutCommonJSInESM
 							}
+
+						case *js_ast.EDot:
+							if target.Name == "exports" {
+								if id, ok := target.Target.Data.(*js_ast.EIdentifier); ok {
+									if symbol := &p.symbols[id.Ref.InnerIndex]; symbol.Kind == js_ast.SymbolUnbound &&
+										symbol.OriginalName == "module" && !symbol.Flags.Has(js_ast.DidWarnAboutCommonJSInESM) {
+										// "module.exports.foo = ..."
+										name = symbol.OriginalName
+										loc = target.Target.Loc
+										symbol.Flags |= js_ast.DidWarnAboutCommonJSInESM
+									}
+								}
+							}
+						}
+
+						if name != "" {
+							kind := logger.Warning
+							if p.suppressWarningsAboutWeirdCode {
+								kind = logger.Debug
+							}
+							p.log.AddWithNotes(kind, &p.tracker, js_lexer.RangeOfIdentifier(p.source, loc),
+								fmt.Sprintf("The CommonJS %q variable is treated as a global variable in an ECMAScript module and may not work as expected", name),
+								p.whyESModule())
 						}
 					}
 				}