diff --git a/src/ast/expr.rs b/src/ast/expr.rs index 3beff11a4014..987ea4b27f4d 100644 --- a/src/ast/expr.rs +++ b/src/ast/expr.rs @@ -2664,9 +2664,8 @@ pub enum Equality { Unknown, Equal, NotEqual, - /// `require.main === module` (or `!==`/`==`/`!=`, in either order); the - /// caller rewrites this to an import_meta_main node. - RequireMainAndModule, + /// `require.main === module` (any equality operator, either order) and the `module` ref. + RequireMainAndModule(Ref), } impl From for Equality { @@ -2703,7 +2702,8 @@ impl EqlKindT for StrictEql { /// blanket-impl'd for every `P<...>` instantiation below. pub trait EqlParser { fn arena(&self) -> &Bump; - fn module_ref(&self) -> Ref; + /// Does this identifier ref name the CommonJS `module` object? + fn is_commonjs_module_ref(&self, ref_: Ref) -> bool; } // `impl EqlParser for P<...>` lives in `bun_js_parser` (next to `P`). @@ -2834,8 +2834,8 @@ impl Data { // always re-ordered to the right side. if matches!(right, Data::ERequireMain) { if let Some(id) = left.as_e_identifier() { - if id.ref_.eql(p.module_ref()) { - return Equality::RequireMainAndModule; + if p.is_commonjs_module_ref(id.ref_) { + return Equality::RequireMainAndModule(id.ref_); } } } diff --git a/src/ast/symbol.rs b/src/ast/symbol.rs index 1b76b3abee95..e520254706dd 100644 --- a/src/ast/symbol.rs +++ b/src/ast/symbol.rs @@ -116,6 +116,9 @@ bitflags::bitflags! { /// Renaming can also break any identifier used inside a "with" statement. const MUST_NOT_BE_RENAMED = 1 << 2; + /// One "CommonJS variable in an ECMAScript module" warning per unbound `module`/`exports`. + const DID_WARN_ABOUT_COMMONJS_IN_ESM = 1 << 3; + const REMOVE_OVERWRITTEN_FUNCTION_DECLARATION = 1 << 4; /// Used in HMR to decide when live binding code is needed. @@ -143,6 +146,7 @@ macro_rules! symbol_flag_accessors { symbol_flag_accessors! { must_start_with_capital_letter_for_jsx, set_must_start_with_capital_letter_for_jsx => MUST_START_WITH_CAPITAL_LETTER_FOR_JSX; must_not_be_renamed, set_must_not_be_renamed => MUST_NOT_BE_RENAMED; + did_warn_about_commonjs_in_esm, set_did_warn_about_commonjs_in_esm => DID_WARN_ABOUT_COMMONJS_IN_ESM; remove_overwritten_function_declaration, set_remove_overwritten_function_declaration => REMOVE_OVERWRITTEN_FUNCTION_DECLARATION; has_been_assigned_to, set_has_been_assigned_to => HAS_BEEN_ASSIGNED_TO; } diff --git a/src/js_parser/lib.rs b/src/js_parser/lib.rs index b8f1f973138e..2bbe3ba9ef30 100644 --- a/src/js_parser/lib.rs +++ b/src/js_parser/lib.rs @@ -247,9 +247,16 @@ impl<'a, const IS_TS: bool, const SCAN: bool> bun_ast::expr::EqlParser fn arena(&self) -> &bun_alloc::Arena { self.arena } - #[inline] - fn module_ref(&self) -> Ref { - self.module_ref + fn is_commonjs_module_ref(&self, ref_: Ref) -> bool { + if ref_.eql(self.module_ref) { + return true; + } + if !ref_.is_symbol() { + return false; + } + // An unbound `module` (file with ESM exports) still gets the `import.meta.main` rewrite. + let symbol = &self.symbols[ref_.inner_index() as usize]; + symbol.kind == bun_ast::symbol::Kind::Unbound && symbol.original_name.slice() == b"module" } } diff --git a/src/js_parser/p.rs b/src/js_parser/p.rs index 12d485079d93..b1996edaf0c7 100644 --- a/src/js_parser/p.rs +++ b/src/js_parser/p.rs @@ -269,6 +269,9 @@ pub struct P<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> { // Used for forcing CommonJS pub(crate) has_with_scope: bool, + /// `export`/top-level `await` in the source, fixed before unwrapping `exports.foo` adds one. + pub(crate) has_esm_exports_syntax: bool, + /// `has_esm_exports_syntax` or an ESM module type (`.mjs`, `"type": "module"`). pub(crate) is_file_considered_to_have_esm_exports: bool, pub(crate) has_called_runtime: bool, @@ -1438,6 +1441,70 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O self.commonjs_named_exports_deoptimized && self.commonjs_named_exports.count() > 0 } + /// Warns once per unbound `module`/`exports` that `left` (an assignment target) writes to. + pub(crate) fn warn_about_commonjs_variable_in_esm(&mut self, left: &Expr) { + let Some(dot) = left.data.e_dot() else { + return; + }; + let (id, loc, is_module_exports_property) = match dot.target.data { + // "module.exports = ..." and "exports.foo = ..." + js_ast::ExprData::EIdentifier(id) => (id, dot.target.loc, false), + // "module.exports.foo = ..." + js_ast::ExprData::EDot(inner) if inner.name == b"exports" => match inner.target.data { + js_ast::ExprData::EIdentifier(id) => (id, inner.target.loc, true), + _ => return, + }, + _ => return, + }; + if !id.ref_.is_symbol() { + return; + } + + let symbol = &mut self.symbols[id.ref_.inner_index() as usize]; + if symbol.kind != js_ast::symbol::Kind::Unbound || symbol.did_warn_about_commonjs_in_esm() { + return; + } + let name = symbol.original_name.slice(); + let is_commonjs_variable = if is_module_exports_property { + name == b"module" + } else { + (name == b"module" && dot.name == b"exports") || name == b"exports" + }; + if !is_commonjs_variable { + return; + } + symbol.set_did_warn_about_commonjs_in_esm(true); + + let (why_range, why_text) = self.why_esm_note(); + let source = self.source; + self.log().add_range_warning_fmt_with_note( + Some(source), + js_lexer::range_of_identifier(source, loc), + format_args!( + "The CommonJS \"{}\" variable is treated as a global variable in an ECMAScript module and may not work as expected", + bstr::BStr::new(name) + ), + format_args!("{}", why_text), + why_range, + ); + } + + /// Diagnostic note pointing at the syntax that makes this file an ES module. + pub(crate) fn why_esm_note(&self) -> (bun_ast::Range, &'static str) { + debug_assert!(self.has_esm_exports_syntax); + if self.esm_export_keyword.len > 0 { + ( + self.esm_export_keyword, + "This file is considered to be an ECMAScript module because of the \"export\" keyword here:", + ) + } else { + ( + self.top_level_await_keyword, + "This file is considered to be an ECMAScript module because of the top-level \"await\" keyword here:", + ) + } + } + pub(crate) fn record_usage(&mut self, ref_: Ref) { if self.is_revisit_for_substitution { return; @@ -2642,9 +2709,10 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O self.scope_order_to_visit = buf.into_bump_slice(); } - self.is_file_considered_to_have_esm_exports = !self.top_level_await_keyword.is_empty() - || !self.esm_export_keyword.is_empty() - || self.options.module_type == options::ModuleType::Esm; + self.has_esm_exports_syntax = + !self.top_level_await_keyword.is_empty() || !self.esm_export_keyword.is_empty(); + self.is_file_considered_to_have_esm_exports = + self.has_esm_exports_syntax || self.options.module_type == options::ModuleType::Esm; self.push_scope_for_visit_pass(js_ast::scope::Kind::Entry, loc_module_scope)?; self.fn_or_arrow_data_visit.is_outside_fn_or_arrow = true; @@ -2744,10 +2812,17 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O .generated .ensure_unused_capacity(generated_symbols_count as usize * 3); - self.exports_ref = - self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"exports")?; - self.module_ref = - self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"module")?; + // `require` stays usable in an ES module; `module` and `exports` do not. + if !self.has_esm_exports_syntax { + self.exports_ref = + self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"exports")?; + self.module_ref = + self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"module")?; + } else { + // Not in scope: user code gets globals. `exports_ref` is still the namespace object. + self.exports_ref = self.new_symbol(js_ast::symbol::Kind::Hoisted, b"exports"); + self.module_ref = self.new_symbol(js_ast::symbol::Kind::Hoisted, b"module"); + } self.require_ref = self.declare_common_js_symbol(js_ast::symbol::Kind::Unbound, b"require")?; @@ -8687,6 +8762,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O macro_call_count: 0, hoisted_ref_for_sloppy_mode_block_fn: Default::default(), has_with_scope: false, + has_esm_exports_syntax: false, is_file_considered_to_have_esm_exports: false, has_called_runtime: false, symbol_uses, diff --git a/src/js_parser/parse/parse_entry.rs b/src/js_parser/parse/parse_entry.rs index cdb4d149ceec..4c31ad49abb7 100644 --- a/src/js_parser/parse/parse_entry.rs +++ b/src/js_parser/parse/parse_entry.rs @@ -1574,9 +1574,14 @@ impl<'a> Parser<'a> { // Checked after `to_ast`, which marks TypeScript type-only imports unused. let mut reject_import_statements = false; - if p.is_deoptimized_commonjs() { + if p.has_esm_exports_syntax { + // `module`/`exports` were never bound here, so nothing CommonJS was recorded. + debug_assert!(!p.is_deoptimized_commonjs()); + exports_kind = js_ast::ExportsKind::Esm; + } else if p.is_deoptimized_commonjs() { exports_kind = js_ast::ExportsKind::Cjs; - } else if p.esm_export_keyword.len > 0 || p.top_level_await_keyword.len > 0 { + } else if p.esm_export_keyword.len > 0 { + // Synthesized by unwrapping `exports.foo = ...` to an ESM export. exports_kind = js_ast::ExportsKind::Esm; } else if uses_exports_ref || uses_module_ref || p.has_top_level_return || p.has_with_scope { diff --git a/src/js_parser/repl_transforms.rs b/src/js_parser/repl_transforms.rs index 06fcfb00fd57..af47b2282557 100644 --- a/src/js_parser/repl_transforms.rs +++ b/src/js_parser/repl_transforms.rs @@ -26,11 +26,6 @@ impl<'a, const TS: bool, const SCAN: bool> P<'a, TS, SCAN> { parts: &mut BumpVec<'bump, js_ast::Part>, bump: &'bump Bump, ) -> Result<(), bun_alloc::AllocError> { - // Skip transform if there's a top-level return (indicates module pattern) - if self.has_top_level_return { - return Ok(()); - } - // Collect all statements let mut total_stmts_count: usize = 0; for part in parts.iter() { diff --git a/src/js_parser/visit/visit_binary.rs b/src/js_parser/visit/visit_binary.rs index e86ba8da5398..e1e26486f550 100644 --- a/src/js_parser/visit/visit_binary.rs +++ b/src/js_parser/visit/visit_binary.rs @@ -236,9 +236,9 @@ impl BinaryExpressionVisitor { } Op::Code::BinLooseEq => { match data_eql::(&e_.left.data, &e_.right.data, p) { - Equality::RequireMainAndModule => { + Equality::RequireMainAndModule(module_ref) => { p.ignore_usage_of_runtime_require(); - p.ignore_usage(p.module_ref); + p.ignore_usage(module_ref); return p.value_for_import_meta_main(false, v.loc); } Equality::Equal => return p.new_expr(E::Boolean { value: true }, v.loc), @@ -266,8 +266,8 @@ impl BinaryExpressionVisitor { } Op::Code::BinStrictEq => { match data_eql::(&e_.left.data, &e_.right.data, p) { - Equality::RequireMainAndModule => { - p.ignore_usage(p.module_ref); + Equality::RequireMainAndModule(module_ref) => { + p.ignore_usage(module_ref); p.ignore_usage_of_runtime_require(); return p.value_for_import_meta_main(false, v.loc); } @@ -289,8 +289,8 @@ impl BinaryExpressionVisitor { } Op::Code::BinLooseNe => { match data_eql::(&e_.left.data, &e_.right.data, p) { - Equality::RequireMainAndModule => { - p.ignore_usage(p.module_ref); + Equality::RequireMainAndModule(module_ref) => { + p.ignore_usage(module_ref); p.ignore_usage_of_runtime_require(); return p.value_for_import_meta_main(true, v.loc); } @@ -316,8 +316,8 @@ impl BinaryExpressionVisitor { } Op::Code::BinStrictNe => { match data_eql::(&e_.left.data, &e_.right.data, p) { - Equality::RequireMainAndModule => { - p.ignore_usage(p.module_ref); + Equality::RequireMainAndModule(module_ref) => { + p.ignore_usage(module_ref); p.ignore_usage_of_runtime_require(); return p.value_for_import_meta_main(true, v.loc); } @@ -674,6 +674,11 @@ impl BinaryExpressionVisitor { was_anonymous_named_expr, ); } + + // Only `=` warns, as in esbuild; `typeof module` is a valid CommonJS check. + if p.options.bundle && p.has_esm_exports_syntax && !p.is_control_flow_dead { + p.warn_about_commonjs_variable_in_esm(&e_.left); + } } Op::Code::BinNullishCoalescingAssign | Op::Code::BinLogicalOrAssign => { // Special case `{}.field ??= value` to minify to `value` diff --git a/src/js_parser/visit/visit_stmt.rs b/src/js_parser/visit/visit_stmt.rs index 2b3cd18271bc..b2d01e09b62f 100644 --- a/src/js_parser/visit/visit_stmt.rs +++ b/src/js_parser/visit/visit_stmt.rs @@ -1531,20 +1531,19 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O ) -> Result<(), Error> { // Forbid top-level return inside modules with ECMAScript-style exports if p.fn_or_arrow_data_visit.is_outside_fn_or_arrow { - let where_ = if p.esm_export_keyword.len > 0 { - p.esm_export_keyword - } else if p.top_level_await_keyword.len > 0 { - p.top_level_await_keyword - } else { - bun_ast::Range::NONE - }; - - if where_.len > 0 { - p.log().add_range_error( + if p.has_esm_exports_syntax { + let (why_range, why_text) = p.why_esm_note(); + p.log().add_range_error_fmt_with_note( Some(p.source), - where_, - b"Top-level return cannot be used inside an ECMAScript module", + js_lexer::range_of_identifier(p.source, stmt.loc), + format_args!("Top-level return cannot be used inside an ECMAScript module"), + format_args!("{}", why_text), + why_range, ); + } else { + p.has_top_level_return = true; + // The wrapper this `return` needs rules out unwrapping `exports.foo = ...` to ESM. + p.deoptimize_common_js_named_exports(); } } diff --git a/src/jsc/RuntimeTranspilerCache.rs b/src/jsc/RuntimeTranspilerCache.rs index 597bcd6952aa..2c5c32368cf2 100644 --- a/src/jsc/RuntimeTranspilerCache.rs +++ b/src/jsc/RuntimeTranspilerCache.rs @@ -55,7 +55,8 @@ bun_core::declare_scope!(cache, visible); /// offsets picked by a header byte) plus a body of tagged records with /// u8/u16/u32 ids and implied slots dropped, instead of fixed u32 arrays. /// Version 27: ModuleInfo string table holds Latin-1 / UTF-16 bodies, not WTF-8. -const EXPECTED_VERSION: u32 = 27; +/// Version 28: A top-level `return` makes a file CommonJS; older entries hold its ESM output. +const EXPECTED_VERSION: u32 = 28; /// Source files smaller than this are not written to / read from the on-disk /// transpiler cache. Originally 50 KiB, which excluded almost every file in a diff --git a/test/bundler/bundler_cjs.test.ts b/test/bundler/bundler_cjs.test.ts index aee7fb02d9fd..f4587bfc00a0 100644 --- a/test/bundler/bundler_cjs.test.ts +++ b/test/bundler/bundler_cjs.test.ts @@ -597,4 +597,358 @@ describe("bundler", () => { stdout: "loaded ok", }, }); + + // ============================================================================ + // A file with `export` (or top-level await) is an ECMAScript module, even if + // it also assigns to `module.exports` or `exports.foo`. In such a file `module` + // and `exports` are plain globals, not the CommonJS wrapper bindings, so the + // ESM exports stay and the bundler warns about the CommonJS assignment. + // ============================================================================ + + const mixedLib = /* js */ ` + export const a = 1; + export function fa() { return a } + export default 9; + if (typeof module !== "undefined") module.exports.b = 2; + `; + const mixedEntry = /* js */ ` + import def, { a, fa } from './lib.js'; + import * as ns from './lib.js'; + console.log(JSON.stringify({ a, fa: typeof fa, def, keys: Object.keys(ns) })); + `; + const mixedStdout = '{"a":1,"fa":"function","def":9,"keys":["a","default","fa"]}'; + const moduleIsGlobalWarning = + 'The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected'; + + for (const format of ["esm", "cjs", "iife"] as const) { + itBundled(`cjs/MixedExportAndModuleExportsIsESM_${format}`, { + files: { + "/entry.js": mixedEntry, + "/lib.js": mixedLib, + }, + format, + bundleWarnings: { + "/lib.js": [moduleIsGlobalWarning], + }, + onAfterBundle(api) { + // lib.js is not wrapped as CommonJS and `module` is printed as the global + api.expectFile("/out.js").not.toContain("__commonJS"); + api.expectFile("/out.js").not.toContain("module_lib"); + api.expectFile("/out.js").toContain("module.exports.b = 2"); + }, + run: { stdout: mixedStdout }, + }); + } + + itBundled("cjs/MixedExportAndModuleExportsNamedImportOfCommonJSProperty", { + files: { + "/entry.js": /* js */ ` + import { a, b } from './lib.js'; + console.log(a, b); + `, + "/lib.js": mixedLib, + }, + bundleWarnings: { + "/lib.js": [moduleIsGlobalWarning], + }, + bundleErrors: { + "/entry.js": ['No matching export in "lib.js" for import "b"'], + }, + }); + + itBundled("cjs/MixedExportAndExportsAssignment", { + files: { + "/entry.js": /* js */ ` + import { a } from './lib.js'; + console.log(a); + `, + "/lib.js": /* js */ ` + export const a = 1; + if (typeof exports !== "undefined") exports.b = 2; + if (typeof module !== "undefined") module.exports = { c: 3 }; + `, + }, + bundleWarnings: { + "/lib.js": [ + 'The CommonJS "exports" variable is treated as a global variable in an ECMAScript module and may not work as expected', + moduleIsGlobalWarning, + ], + }, + onAfterBundle(api) { + api.expectFile("/out.js").not.toContain("__commonJS"); + api.expectFile("/out.js").toContain("exports.b = 2"); + api.expectFile("/out.js").toContain("module.exports = { c: 3 }"); + }, + run: { stdout: "1" }, + }); + + itBundled("cjs/TopLevelAwaitWithModuleExports", { + files: { + "/entry.js": /* js */ ` + await Promise.resolve(); + module.exports = { a: 1 }; + console.log("done"); + `, + }, + bundleWarnings: { + "/entry.js": [moduleIsGlobalWarning], + }, + onAfterBundle(api) { + api.expectFile("/out.js").not.toContain("module_entry"); + api.expectFile("/out.js").toContain("module.exports = { a: 1 }"); + }, + run: { error: "ReferenceError: module is not defined" }, + }); + + itBundled("cjs/TypeofModuleAndExportsInESM", { + files: { + "/entry.js": /* js */ ` + export const x = 1; + console.log(typeof module, typeof exports); + `, + }, + onAfterBundle(api) { + api.expectFile("/out.js").not.toContain("module_entry"); + api.expectFile("/out.js").toContain("typeof module, typeof exports"); + }, + run: { stdout: "undefined undefined" }, + }); + + itBundled("cjs/RequireMainEqualsModuleInESM", { + files: { + "/entry.js": /* js */ ` + export const x = 1; + console.log(require.main === module, require.main !== module); + `, + }, + target: "bun", + onAfterBundle(api) { + api.expectFile("/out.js").not.toContain("module_entry"); + api.expectFile("/out.js").toContain("import.meta.main"); + }, + run: { stdout: "true false" }, + }); + + // Reads of `module.id`, `module.require()` and `exports.foo` in an ESM file + // are printed as written. The CommonJS folds (`module.id` inlined, + // `module.require(x)` rewritten to `require(x)`) do not apply to globals. + itBundled("cjs/ModuleMembersInESMStayVerbatim", { + files: { + "/entry.ts": /* ts */ ` + import { describeEnv } from './lib'; + export const env = describeEnv(); + console.log(env); + `, + "/lib.ts": /* ts */ ` + globalThis['ca' + 'pture'] = x => x; + declare const module: any; + declare const exports: any; + + export function describeEnv() { + return [ + typeof module === 'undefined' ? 'no module' : capture(module.id), + typeof module === 'undefined' ? 'no require' : capture(module.require('node:fs')), + typeof exports === 'undefined' ? 'no exports' : capture(exports.foo), + ].join(', '); + } + `, + }, + capture: ["module.id", 'module.require("node:fs")', "exports.foo"], + run: { stdout: "no module, no require, no exports" }, + }); + + // The renamer must not take the names `module` and `exports` for minified + // identifiers in a file where they are globals. + itBundled("cjs/TypeofModuleAndExportsInESMMinified", { + files: { + "/entry.ts": /* ts */ ` + globalThis['ca' + 'pture'] = x => x; + await Promise.resolve(); + console.log(capture(typeof module), capture(typeof exports)); + `, + }, + minifyIdentifiers: true, + capture: ["typeof module", "typeof exports"], + run: { stdout: "undefined undefined" }, + }); + + // With the CommonJS output format the globals refer to the output file's own + // `module` and `exports`, under bun and under node. + itBundled("cjs/ModuleAndExportsInESMWithFormatCJS", { + files: { + "/entry.ts": /* ts */ ` + import { install } from './install'; + install(); + `, + "/install.ts": /* ts */ ` + declare const module: any; + declare const exports: any; + + export function install() { + console.log(typeof module, typeof exports, module.exports === exports); + } + `, + }, + format: "cjs", + target: "node", + onAfterBundle(api) { + api.expectFile("/out.js").not.toMatch(/\b(?:module|exports)_install\b/); + }, + run: [{ stdout: "object object true" }, { runtime: "node", stdout: "object object true" }], + }); + + // `define` only replaces free identifiers, so it applies to `module` in a file + // with ESM exports and not in a CommonJS file, where `module` is the wrapper's + // argument. + itBundled("cjs/DefineModuleAppliesOnlyInESMFile", { + files: { + "/entry.js": /* js */ ` + import { esm } from './esm.js'; + const cjs = require('./cjs.js'); + console.log(esm, cjs.value); + `, + "/esm.js": /* js */ ` + export const esm = module; + `, + "/cjs.js": /* js */ ` + module.exports = { value: typeof module }; + `, + }, + define: { module: '"defined"' }, + run: { stdout: "defined object" }, + }); + + // Bun keeps its content-based rule: a file with only CommonJS syntax is + // CommonJS, whatever its extension or the package "type" says. + itBundled("cjs/ModuleExportsInMJSStaysCommonJS", { + files: { + "/entry.js": /* js */ ` + import lib from './lib.mjs'; + console.log(JSON.stringify(lib)); + `, + "/lib.mjs": /* js */ ` + module.exports = { a: 1 }; + `, + "/package.json": `{ "type": "module" }`, + }, + onAfterBundle(api) { + api.expectFile("/out.js").toContain("__commonJS"); + }, + run: { stdout: '{"a":1}' }, + }); + + // ============================================================================ + // Top-level `return` is only legal inside the CommonJS function wrapper, so + // its presence makes the file CommonJS when no ESM export syntax is present. + // ============================================================================ + + for (const target of ["browser", "bun", "node"] as const) { + itBundled(`cjs/TopLevelReturnEntry_${target}`, { + files: { + "/entry.js": /* js */ ` + console.log("before"); + if (globalThis.foo === undefined) return; + console.log("after"); + `, + }, + target, + onAfterBundle(api) { + api.expectFile("/out.js").toContain("__commonJS"); + }, + run: { stdout: "before" }, + }); + } + + // With the IIFE format the `return` must stay inside the wrapper; a bare + // `return` at the IIFE's top level would exit the whole bundle. + itBundled("cjs/TopLevelReturnIIFE", { + files: { + "/entry.js": /* js */ ` + console.log("before"); + if (globalThis.foo === undefined) return; + console.log("after"); + `, + }, + format: "iife", + onAfterBundle(api) { + api.expectFile("/out.js").toMatch(/__commonJS\(function\(\) \{[^]*\breturn;[^]*\}\);/); + }, + }); + + itBundled("cjs/TopLevelReturnImported", { + files: { + "/entry.js": /* js */ ` + import "./lib.js"; + console.log("entry"); + `, + "/lib.js": /* js */ ` + console.log("lib: before"); + if (1) return; + console.log("lib: after"); + `, + }, + onAfterBundle(api) { + api.expectFile("/out.js").toContain("__commonJS"); + }, + run: { stdout: "lib: before\nentry" }, + }); + + itBundled("cjs/TopLevelReturnNestedBlock", { + files: { + "/entry.js": /* js */ ` + console.log("A"); + { + if (1) return; + } + console.log("B"); + `, + }, + run: { stdout: "A" }, + }); + + itBundled("cjs/ReturnInsideFunctionIsNotTopLevel", { + files: { + "/entry.js": /* js */ ` + function f() { return 1; } + const g = () => { return 2; }; + console.log(f() + g()); + `, + }, + onAfterBundle(api) { + api.expectFile("/out.js").not.toContain("__commonJS"); + }, + run: { stdout: "3" }, + }); + + // `exports.foo = ...` is normally unwrapped to an ESM export. A top-level + // return needs the CommonJS wrapper, so the unwrapping must be undone. + itBundled("cjs/TopLevelReturnKeepsExportsAssignments", { + files: { + "/entry.js": /* js */ ` + const lib = require('./lib.js'); + console.log(JSON.stringify(lib)); + `, + "/lib.js": /* js */ ` + exports.foo = 1; + if (globalThis.skip) return; + exports.bar = 2; + `, + }, + onAfterBundle(api) { + api.expectFile("/out.js").toContain("__commonJS"); + }, + run: { stdout: '{"foo":1,"bar":2}' }, + }); + + itBundled("cjs/TopLevelReturnWithExportIsAnError", { + files: { + "/entry.js": /* js */ ` + export const a = 1; + return; + `, + }, + bundleErrors: { + "/entry.js": ["Top-level return cannot be used inside an ECMAScript module"], + }, + }); }); diff --git a/test/bundler/esbuild/default.test.ts b/test/bundler/esbuild/default.test.ts index 950319df7bab..90be2bdf0a7f 100644 --- a/test/bundler/esbuild/default.test.ts +++ b/test/bundler/esbuild/default.test.ts @@ -1545,7 +1545,6 @@ describe.concurrent("bundler", () => { }, }); itBundled("default/TopLevelReturnForbiddenImport", { - todo: true, files: { "/entry.js": /* js */ ` console.log('A'); @@ -1563,7 +1562,6 @@ describe.concurrent("bundler", () => { }, }); itBundled("default/TopLevelReturnForbiddenImportAndModuleExports", { - todo: true, files: { "/entry.js": /* js */ ` module.exports.foo = 123 @@ -5653,11 +5651,12 @@ describe.concurrent("bundler", () => { }, entryPoints: ["/cjs-in-esm.js", "/import-in-cjs.js", "/no-warnings-here.js"], format: "cjs", - /* TODO FIX expectedScanLog: `cjs-in-esm.js: WARNING: The CommonJS "exports" 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-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: - `, */ + bundleWarnings: { + "/cjs-in-esm.js": [ + 'The CommonJS "exports" variable is treated as a global variable in an ECMAScript module and may not work as expected', + 'The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected', + ], + }, external: ["bar"], }); itBundled("default/MangleProps", { diff --git a/test/bundler/transpiler/transpiler.test.js b/test/bundler/transpiler/transpiler.test.js index 590362557892..50f5e2de9468 100644 --- a/test/bundler/transpiler/transpiler.test.js +++ b/test/bundler/transpiler/transpiler.test.js @@ -3060,6 +3060,34 @@ console.log(resolve.length) expectPrinted_("Math.log('hi')", 'console.error("hi")'); }); + it("define replaces module/exports only where they are not CommonJS bindings", () => { + const defineModule = new Bun.Transpiler({ + loader: "ts", + define: { module: '"defined-module"', exports: '"defined-exports"' }, + }); + const print = code => defineModule.transformSync(code).trim(); + + // A file with an `export` or top-level `await` has no `module`/`exports` bindings. + expect(print("export const a = [module, exports];")).toBe( + 'export const a = ["defined-module", "defined-exports"];', + ); + expect(print("await 0;\nconsole.log(module, exports);")).toBe( + 'await 0;\nconsole.log("defined-module", "defined-exports");', + ); + + // A CommonJS file binds them to the wrapper's arguments, so the define does not apply. + expect(print("console.log(module, exports);")).toBe("console.log(module, exports);"); + expect(print("import x from './x';\nconsole.log(module, exports, x);")).toBe( + 'import x from "./x";\nconsole.log(module, exports, x);', + ); + }); + + it("module.require() is only rewritten to require() where module is a CommonJS binding", () => { + expectPrinted_("const x = module.require('y')", 'const x = require("y")'); + expectPrinted_("export const x = module.require('y')", 'export const x = module.require("y")'); + expectPrinted_("await 0;\nconst x = module.require('y')", 'await 0;\nconst x = module.require("y")'); + }); + it("jsx symbol should work", () => { expectBunPrinted_(`var x = jsx; export default x;`, "var x = jsx;\nexport default x"); }); diff --git a/test/cli/run/esm-cjs-detection.test.ts b/test/cli/run/esm-cjs-detection.test.ts new file mode 100644 index 000000000000..11094a531f0e --- /dev/null +++ b/test/cli/run/esm-cjs-detection.test.ts @@ -0,0 +1,362 @@ +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; +import { join } from "path"; + +// How a file is classified as CommonJS or ESM, and that `bun run` and +// `bun build` agree on it: +// - `export` or top-level `await` makes the file ESM. `module` and `exports` +// are then plain globals, whatever else the file does with them. +// - Otherwise `module`, `exports`, or a top-level `return` make the file +// CommonJS. The extension and the package "type" do not override the file's +// own syntax. + +interface Result { + stdout: string; + stderr: string; + exitCode: number; +} + +async function bun(cwd: string, args: string[]): Promise { + await using proc = Bun.spawn({ + cmd: [bunExe(), ...args], + env: bunEnv, + cwd, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +type Format = "esm" | "cjs" | "iife"; + +/** `bun build --format= ` into `out/.js`, then run that file. */ +async function buildAndRun(cwd: string, entry: string, format: Format): Promise<{ build: Result; run: Result }> { + const outfile = join("out", `${format}.js`); + const build = await bun(cwd, ["build", entry, `--format=${format}`, `--outfile=${outfile}`]); + if (build.exitCode !== 0) { + return { build, run: { stdout: "", stderr: "", exitCode: -1 } }; + } + return { build, run: await bun(cwd, [outfile]) }; +} + +const mixedLib = /* js */ ` + export const a = 1; + export function fa() { return a } + export default 9; + if (typeof module !== "undefined") module.exports.b = 2; +`; +const mixedEntry = (lib: string) => /* js */ ` + import def, { a, fa } from "./${lib}"; + import * as ns from "./${lib}"; + console.log(JSON.stringify({ a, fa: typeof fa, def, keys: Object.keys(ns) })); +`; +const mixedStdout = '{"a":1,"fa":"function","def":9,"keys":["a","default","fa"]}\n'; +const moduleIsGlobalWarning = + 'The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected'; + +const packageTypes = { + none: null, + module: `{ "type": "module" }`, + commonjs: `{ "type": "commonjs" }`, +}; + +function withPackageJson(files: Record, packageType: keyof typeof packageTypes) { + const packageJson = packageTypes[packageType]; + return packageJson === null ? files : { ...files, "package.json": packageJson }; +} + +describe.concurrent("a file with export and module.exports is ESM", () => { + test("bun run and every bundle format see the same exports", async () => { + using dir = tempDir("esm-cjs-mixed", { + "entry.js": mixedEntry("lib.js"), + "lib.js": mixedLib, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe(mixedStdout); + expect(source.exitCode).toBe(0); + + for (const format of ["esm", "cjs", "iife"] as const) { + const { build, run } = await buildAndRun(cwd, "entry.js", format); + expect(build.stderr).toContain(moduleIsGlobalWarning); + expect(build.stderr).toContain('because of the "export" keyword here'); + expect(build.exitCode).toBe(0); + expect(run.stderr).toBe(""); + expect(run.stdout).toBe(mixedStdout); + expect(run.exitCode).toBe(0); + } + }); + + test("a named import of a module.exports property fails in both", async () => { + using dir = tempDir("esm-cjs-mixed-named", { + "entry.js": /* js */ ` + import { a, b } from "./lib.js"; + console.log(a, b); + `, + "lib.js": mixedLib, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toContain("Export named 'b' not found in module"); + expect(source.stdout).toBe(""); + expect(source.exitCode).toBe(1); + + const build = await bun(cwd, ["build", "entry.js", "--outfile=out/esm.js"]); + expect(build.stderr).toContain('No matching export in "lib.js" for import "b"'); + expect(build.exitCode).toBe(1); + }); + + test("an unguarded module.exports throws ReferenceError in both", async () => { + using dir = tempDir("esm-cjs-mixed-throws", { + "lib.js": /* js */ ` + export const a = 1; + module.exports.b = 2; + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "lib.js"]); + expect(source.stderr).toContain("ReferenceError: module is not defined"); + expect(source.exitCode).toBe(1); + + const { build, run } = await buildAndRun(cwd, "lib.js", "esm"); + expect(build.stderr).toContain(moduleIsGlobalWarning); + expect(run.stderr).toContain("ReferenceError: module is not defined"); + expect(run.exitCode).toBe(1); + }); + + test("module and exports are globals: typeof reports the output file's environment", async () => { + using dir = tempDir("esm-cjs-typeof", { + "entry.js": /* js */ ` + export const x = 1; + console.log(typeof module, typeof exports); + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe("undefined undefined\n"); + + // The CommonJS output file has a real `module` and `exports`. The IIFE output + // has no ESM syntax and mentions both, so `bun` runs it as CommonJS too. + const expected = { esm: "undefined undefined\n", cjs: "object object\n", iife: "object object\n" }; + for (const format of ["esm", "cjs", "iife"] as const) { + const { build, run } = await buildAndRun(cwd, "entry.js", format); + expect(build.stderr).not.toContain("warn:"); + expect(run.stderr).toBe(""); + expect(run.stdout).toBe(expected[format]); + } + }); + + test("top-level await with module.exports", async () => { + using dir = tempDir("esm-cjs-tla", { + "entry.js": /* js */ ` + await Promise.resolve(); + module.exports = { a: 1 }; + console.log("done"); + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toContain("ReferenceError: module is not defined"); + expect(source.exitCode).toBe(1); + + const { build, run } = await buildAndRun(cwd, "entry.js", "esm"); + expect(build.stderr).toContain(moduleIsGlobalWarning); + expect(build.stderr).toContain('because of the top-level "await" keyword here'); + expect(run.stderr).toContain("ReferenceError: module is not defined"); + expect(run.exitCode).toBe(1); + }); + + test("require.main === module still becomes import.meta.main", async () => { + using dir = tempDir("esm-cjs-require-main", { + "entry.js": /* js */ ` + export const x = 1; + console.log(require.main === module); + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe("true\n"); + + const build = await bun(cwd, ["build", "entry.js", "--target=bun", "--outfile=out/esm.js"]); + expect(build.stderr).not.toContain("warn:"); + expect(build.exitCode).toBe(0); + const run = await bun(cwd, ["out/esm.js"]); + expect(run.stderr).toBe(""); + expect(run.stdout).toBe("true\n"); + }); + + for (const ext of ["js", "mjs", "cjs"] as const) { + for (const packageType of ["none", "module", "commonjs"] as const) { + test(`the export keyword wins in lib.${ext} with package type ${packageType}`, async () => { + using dir = tempDir( + "esm-cjs-mixed-matrix", + withPackageJson( + { + "entry.js": mixedEntry(`lib.${ext}`), + [`lib.${ext}`]: mixedLib, + }, + packageType, + ), + ); + const source = await bun(String(dir), ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe(mixedStdout); + expect(source.exitCode).toBe(0); + }); + } + } +}); + +describe.concurrent("a top-level return makes a file CommonJS", () => { + const topLevelReturn = /* js */ ` + console.log(1); + if (globalThis.foo === undefined) return; + console.log(2); + `; + + test("bun run and the bundle agree", async () => { + using dir = tempDir("esm-cjs-return", { "entry.js": topLevelReturn }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe("1\n"); + expect(source.exitCode).toBe(0); + + for (const format of ["esm", "cjs"] as const) { + const { build, run } = await buildAndRun(cwd, "entry.js", format); + expect(build.stderr).not.toContain("warn:"); + expect(build.exitCode).toBe(0); + expect(run.stderr).toBe(""); + expect(run.stdout).toBe("1\n"); + expect(run.exitCode).toBe(0); + } + }); + + for (const ext of ["js", "mjs", "cjs"] as const) { + for (const packageType of ["none", "module", "commonjs"] as const) { + test(`entry.${ext} with package type ${packageType}`, async () => { + using dir = tempDir( + "esm-cjs-return-matrix", + withPackageJson({ [`entry.${ext}`]: topLevelReturn }, packageType), + ); + const source = await bun(String(dir), ["run", `entry.${ext}`]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe("1\n"); + expect(source.exitCode).toBe(0); + }); + } + } + + test("a required module keeps its exports.foo assignments around the return", async () => { + using dir = tempDir("esm-cjs-return-exports", { + "main.js": /* js */ ` + const lib = require("./lib.js"); + console.log(JSON.stringify(lib)); + `, + "lib.js": /* js */ ` + exports.foo = 1; + if (globalThis.skip) return; + exports.bar = 2; + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "main.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe('{"foo":1,"bar":2}\n'); + + const { build, run } = await buildAndRun(cwd, "main.js", "esm"); + expect(build.stderr).not.toContain("warn:"); + expect(build.exitCode).toBe(0); + expect(run.stderr).toBe(""); + expect(run.stdout).toBe('{"foo":1,"bar":2}\n'); + }); + + test("a top-level return next to an export is an error in both", async () => { + using dir = tempDir("esm-cjs-return-export", { + "entry.js": /* js */ ` + export const a = 1; + return; + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toContain("Top-level return cannot be used inside an ECMAScript module"); + expect(source.exitCode).toBe(1); + + const build = await bun(cwd, ["build", "entry.js", "--outfile=out/esm.js"]); + expect(build.stderr).toContain("Top-level return cannot be used inside an ECMAScript module"); + expect(build.stderr).toContain('because of the "export" keyword here'); + expect(build.exitCode).toBe(1); + }); + + // `typeof arguments` is "object" inside the CommonJS wrapper and "undefined" + // at the top level of an ES module. + test("return inside a function does not make the file CommonJS", async () => { + using dir = tempDir("esm-cjs-return-fn", { + "entry.js": /* js */ ` + function f() { return 1; } + console.log(f(), typeof arguments); + `, + }); + const source = await bun(String(dir), ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe("1 undefined\n"); + }); + + test("a top-level return without any other CommonJS syntax", async () => { + using dir = tempDir("esm-cjs-return-only", { + "entry.js": /* js */ ` + console.log(typeof arguments); + if (globalThis.foo === undefined) return; + `, + }); + const source = await bun(String(dir), ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe("object\n"); + }); +}); + +describe.concurrent("a file with only CommonJS syntax is CommonJS", () => { + for (const [label, files] of [ + ["lib.mjs", { "lib.mjs": `module.exports = { a: 1 };` }], + [ + "lib.js in a type module package", + { "lib.js": `module.exports = { a: 1 };`, "package.json": `{ "type": "module" }` }, + ], + ] as const) { + test(`module.exports in ${label}`, async () => { + const lib = Object.keys(files)[0]; + using dir = tempDir("esm-cjs-cjs-only", { + ...files, + "entry.js": /* js */ ` + import lib from "./${lib}"; + console.log(JSON.stringify(lib)); + `, + }); + const cwd = String(dir); + + const source = await bun(cwd, ["run", "entry.js"]); + expect(source.stderr).toBe(""); + expect(source.stdout).toBe('{"a":1}\n'); + + const { build, run } = await buildAndRun(cwd, "entry.js", "esm"); + expect(build.stderr).not.toContain("warn:"); + expect(build.exitCode).toBe(0); + expect(run.stderr).toBe(""); + expect(run.stdout).toBe('{"a":1}\n'); + }); + } +}); diff --git a/test/js/bun/transpiler/repl-transform.test.ts b/test/js/bun/transpiler/repl-transform.test.ts index 4f811b4762e3..1a8010fac1fd 100644 --- a/test/js/bun/transpiler/repl-transform.test.ts +++ b/test/js/bun/transpiler/repl-transform.test.ts @@ -72,6 +72,11 @@ describe("Bun.Transpiler replMode", () => { expect(result).toContain("console.log(this)"); expect(result).not.toContain("console.log(exports)"); }); + + test("top-level return is still wrapped in the IIFE", () => { + const result = transpiler.transformSync("return 42"); + expect(result.trim()).toBe("(() => {\n return 42;\n})();"); + }); }); describe("REPL session with node:vm", () => {