From 902715722d33606ebae3a3e0bcface88f2e1f4ab Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 5 Jan 2022 21:39:43 +0000 Subject: [PATCH] warn about "module.exports.x = y" in ESM (#1907) --- CHANGELOG.md | 20 ++++++++++ internal/bundler/bundler_default_test.go | 8 ++++ .../bundler/snapshots/snapshots_loader.txt | 10 +++++ internal/js_parser/js_parser.go | 40 ++++++++++++++----- 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37b0623165b..562f05d49b0 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index ae6297e2886..40fb1473a0d 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -5218,10 +5218,15 @@ 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) @@ -5229,6 +5234,7 @@ func TestWarnCommonJSExportsInESMConvert(t *testing.T) { }, 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: `, }) } diff --git a/internal/bundler/snapshots/snapshots_loader.txt b/internal/bundler/snapshots/snapshots_loader.txt index c6f1aeab946..0b077d82bc5 100644 --- a/internal/bundler/snapshots/snapshots_loader.txt +++ b/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); diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 0d40d475550..95566bd3961 100644 --- a/internal/js_parser/js_parser.go +++ b/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()) } } }