[robobun]

Problem

Bundling an ES module that mentions the free identifiers module or exports produces a bundle that references symbols which are never declared, and --define module=... / define: { module } is ignored for it.

// a.ts
declare const module: string;
export function f() { return `mod=${module}`; }
// entry.ts
import { f } from "./a";
console.log(f());

$ bun build ./entry.ts --target=node --format=cjs --outfile=out.js && node out.js
ReferenceError: module_a is not defined

The bundle contains return `mod=${module_a}` ; nothing declares module_a. esbuild emits `mod=${module}` and, with --define:module='"bun"', `mod=${"bun"}`.

More of the same bug in one file (export const a = 1; plus a few references), current output:

console.log(exports_e, exports_e.foo, "e.ts", module_e.require, typeof module_e, typeof exports_e);

module.exports and exports became the file's ESM namespace object (so typeof exports is "object"), module.id was inlined, module_e is undeclared, and with --minify the references became whatever letter the minifier picked (typeof a). A file mixing export with module.exports = {...} was even switched to CommonJS and its export statements silently dropped.

The bun npm package's postinstall (packages/bun-release/scripts/npm-postinstall.ts) hits this when bundled with Bun.build({ define: { module: '"bun"' } }): install.ts ends up with an undeclared module_install.

Cause

prepare_for_visit_pass unconditionally declares the CommonJS exports/module symbols in the module scope, so every bare exports/module in the file binds to exports_ref/module_ref and the CommonJS-specific folds (module.exports rewriting, module.id inlining, named-export unwrapping) run on it. But a file containing an export statement or top-level await is classified ESM no matter what (exports_kind selection in parse_entry.rs), and for ESM files the linker never declares module_ref, reuses exports_ref as the namespace object, and renames both to module_<file> / exports_<file> on the assumption that nothing user-visible refers to them. The define pass never saw the identifiers either, since it only substitutes unbound symbols.

Fix

When the file has an export keyword or top-level await (now the shared is_esm_by_syntax() predicate, which is also what the exports_kind selection in parse_entry.rs and is_file_considered_to_have_esm_exports use), create exports_ref/module_ref without putting them in scope (they are still needed by ref: namespace object, export =, HMR). A bare exports/module then resolves to an ordinary unbound symbol: printed verbatim, reserved by the renamer, and eligible for define. This is what esbuild does (prepareForVisitPass: "You can still use require in ESM, just not module or exports"). The CommonJS folds keyed on those symbols stop applying to such files as a consequence: module.exports rewriting and named-export unwrapping, module.id/filename/path inlining, and the webpack-compat rewrite of module.require(x) to require(x) (so that call is now printed as written in these files). esbuild applies none of them to such files either. Note that this does not change which files count as ESM: for example a TypeScript file with only export type declarations plus module.exports = ... was already classified ESM (and already failed with module is not defined at runtime); it now bundles to a verbatim module.exports instead of an undeclared module_<file>.

options.module_type is deliberately not part of the condition: a .mjs / "type": "module" file without ESM syntax is still classified by its use of module/exports (see also #33807), and that needs the bindings.

Bun's require.main === module -> import.meta.main rewrite is kept for ESM files: EqlParser::is_module_ref now also accepts an unbound identifier named module (which can only exist in a file without the binding), and the four equality arms drop the use of whichever identifier matched instead of always module_ref.

Why this is the right behavior

The binding decision now agrees with the classification decision the parser already makes: whenever the file is going to be ESM regardless of what else it contains, it gets no CommonJS bindings, exactly like the environment it will run in. Free module/exports then mean whatever the output environment provides: undefined in an ESM bundle (so typeof module !== "undefined" dual-mode guards take the ESM path), the real wrapper values in --format=cjs output, and the defined value under define. This matches esbuild for every case above. Files without ESM syntax, import-only files, and TypeScript export = (which resets the export keyword) are untouched and keep CommonJS semantics, so defines for module/exports still do not apply there (covered by the existing extra/DefineModule* tests).

Observable changes outside the bundler, all limited to files that contain export or top-level await and mention module/exports: --define module=.../exports=... now applies to them at runtime and in Bun.Transpiler; a module.require(...) call in such a file is no longer rewritten to require(...) (the rewrite exists for CommonJS files, and Node/esbuild do not apply it to ES modules either); and the dev server no longer binds their module/exports to the closure's placeholder arguments (those were undefined at runtime, so typeof guards behave the same). Because the runtime transpiler output changes, EXPECTED_VERSION in RuntimeTranspilerCache.rs is bumped to 26 so cached entries from older builds are not replayed.

Tests

test/bundler/bundler_edgecase.test.ts:

• ESMFileModuleAndExportsAreFreeIdentifiers: captures typeof module, typeof exports, module.id, module.require("node:fs"), exports.foo verbatim and the require.main === module fold, and runs the ESM bundle. Before: typeof module_lib, typeof exports_lib, "lib.ts", the require rewritten to the bundler's builtin stub, exports_lib.foo.
• ESMFileWithGuardedCommonJSAssignmentsStaysESM: three exporting files with guarded module.exports = ... / exports.x = ... assignments (one file doing both). The assignments are captured verbatim, the output contains no __commonJS wrapper, and the entry imports all three. Before: module_assigns_module_exports.exports = ..., exports_assigns_exports.b = ..., and the file doing both converted to a CommonJS wrapper.
• ...Minified: top-level await file with minifyIdentifiers. Before: typeof a, typeof e.
• ...FormatCJS: --format=cjs --target=node, run under both bun and node; module/exports refer to the output file's own CommonJS module. Before: ReferenceError: module_install is not defined.
• DefineModuleAndExportsInESMFile: define replaces them in the ESM file and not in a CommonJS file in the same bundle. Before: ReferenceError: module_esm is not defined.

test/bundler/transpiler/transpiler.test.js: Bun.Transpiler with define for module/exports substitutes in export / top-level-await sources and leaves CommonJS and import-only sources alone (before: no substitution in the ESM cases), and module.require('y') still prints as require("y") in a CommonJS source but as written in export / top-level-await sources (before: rewritten in all three).

All seven fail on the unpatched binary and pass with this change. Also ran test/bundler/ (the only failures are pre-existing debug-build-only ones unrelated to this change: compile/HelloWorldWithProcessVersionsBun, the native-plugin install hook timeout, and one compile test timeout), plus test/bake/dev/bundle.test.ts, test/bake/dev/esm.test.ts, test/bake/dev-and-prod.test.ts, test/cli/run/run-eval.test.ts, test/cli/run/run-importmetamain.ts, test/js/bun/transpiler/repl-transform.test.ts and test/js/third_party/es-module-lexer, all passing.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts

[robobun]

Status: reproduced with bun build ./entry.ts --format=cjs --target=node on 1.4.0 (ReferenceError: module_a is not defined) and with the seven new tests in test/bundler/bundler_edgecase.test.ts / test/bundler/transpiler/transpiler.test.js, which fail on the unpatched binary and pass with this branch. Fix is in the parser (prepare_for_visit_pass), details in the description.

Ready for review: all review threads are resolved. CI for the current head (build 92195) passed every job that ran (192/192); the build is marked failed only because the four macOS 14 test jobs expired in the agent queue without ever running, exactly as on the first build of this PR (91815), and the same jobs are currently stuck in the queue on unrelated builds too, so this is the CI pool rather than anything in the diff. The change is platform-independent parser logic.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes how the parser binds module/exports in files with ESM syntax — a semantic shift that ripples into the bundler's linker, define substitution, CommonJS-unwrap folds, and the dev server — a human look would still be worthwhile.

What was reviewed:

• Verified the esm_export_keyword || top_level_await_keyword gate in prepare_for_visit_pass matches the unconditional-ESM branch of exports_kind selection in parse_entry.rs, and that is_deoptimized_commonjs() can no longer fire for such files (unbound module/exports never touch module_ref/exports_ref).
• Checked fold_require_main_and_module: is_primitive_to_reorder includes ERequireMain but not EIdentifier, so after the equality reorder the identifier is always e_.left and ignore_usage(id.ref_) drops the correct ref in both the bound (CJS) and unbound (ESM) cases.
• Checked is_module_ref: the unbound-module fallback cannot match a user-declared or shadowed module (those are not Kind::Unbound), and in CJS files no unbound module can exist because declare_common_js_symbol puts the binding in module scope.

Extended reasoning...

Overview

This PR changes prepare_for_visit_pass in src/js_parser/p.rs so that when a file contains an export keyword or top-level await (the same condition under which parse_entry.rs unconditionally classifies it as ESM), the CommonJS exports_ref/module_ref symbols are created via new_symbol instead of declare_common_js_symbol — i.e., they exist for the linker/HMR to reference but are not placed in the module scope. Bare module/exports in such files then resolve as ordinary unbound globals: printed verbatim, reserved by the renamer, and eligible for --define. To keep Bun's require.main === module → import.meta.main rewrite working, EqlParser::module_ref() becomes is_module_ref(ref_) which also accepts an unbound symbol named module, and the four equality arms in visit_binary.rs are deduplicated into fold_require_main_and_module that ignores usage of whichever identifier actually matched.

Security risks

None. This is a bundler/transpiler symbol-resolution change with no security-sensitive surface.

Level of scrutiny

High. The parser's decision about whether module/exports bind to the file's CommonJS refs is load-bearing for the linker (which reuses exports_ref as the ESM namespace object and renames both), for the CommonJS-unwrap folds in fold.rs (module.exports rewriting, module.id inlining, named-export extraction), for is_deoptimized_commonjs(), and for the dev server's HMR wrapper. The change is well-reasoned, mirrors esbuild's prepareForVisitPass, and the PR description explicitly enumerates the downstream effects, but it is a user-visible semantic change (e.g. typeof exports now yields "undefined" instead of "object" in bundled ESM files that reference it, and define: { module } now applies where it previously did not).

Other factors

The tests are thorough: four new itBundled cases cover free-identifier printing, minified output, --format=cjs runtime behavior under both bun and node, and define applying only to the ESM half of a mixed bundle; a Bun.Transpiler test covers all four source shapes (export, TLA, plain CJS, import-only). The PR description states all five fail on the unpatched binary. Existing edgecase/ImportMetaMain*, edgecase/build-cjs-module#20308, and edgecase/IsBuffer2 tests already exercise the require.main === module fold and typeof module/typeof exports in files with ESM syntax, so a regression in the reorder/ignore_usage path or the unbound-symbol fallback would surface there. I traced is_primitive_to_reorder to confirm the identifier always lands on e_.left after reordering, and confirmed new_symbol produces a valid ref for the linker without adding it to the module scope's members. Given the breadth of downstream consumers of exports_ref/module_ref, I'd like a human to confirm the intent — particularly that the observable change to typeof exports in bundled ESM output and to files that previously deoptimized to CommonJS is desired.

[robobun]

On the two behavior changes called out in the review, both are intended:

• typeof exports in a bundled file that has its own export statements now evaluates to whatever the output environment provides ("undefined" in an ESM bundle, "object" with --format=cjs). The old "object" came from binding the user's exports to the file's namespace object, so exports.foo = ... in a dual-mode guard wrote into the namespace object. The unbundled source, Node, and esbuild all take the ESM path for that guard, and edgecase/IsBuffer1/IsBuffer2 (the lodash-es shape) still pass.
• A file that contains export statements and also assigns module.exports was previously switched to CommonJS with its export statements dropped (in ESM output), or emitted an undeclared module_<file> (in CJS output). It now stays ESM, as exports_kind already said it was, and the module.exports assignment is left for the environment to resolve, matching esbuild. Such a file cannot run unbundled in either mode, so the only shapes that exist in practice are the guarded ones from the first point.

export = files, import-only files, and files with no ESM syntax are unaffected; extra/DefineModule* and default/ExportsAndModuleFormatCommonJS cover those and still pass.

[robobun]

^{Updated 11:05 AM PT - Aug 11th, 2026}

❌ @robobun, your commit f7374e2 has some failures in Build #92195 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37371

That installs a local version of the PR into your bun-37371 executable, so you can run:

bun-37371 --bun

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c532f3f1-b8e4-4a89-8e0b-37582ef29355

📥 Commits

Reviewing files that changed from the base of the PR and between 81cfca9 and f7374e2.

📒 Files selected for processing (8)

• src/ast/expr.rs
• src/js_parser/lib.rs
• src/js_parser/p.rs
• src/js_parser/parse/parse_entry.rs
• src/js_parser/visit/visit_binary.rs
• src/jsc/RuntimeTranspilerCache.rs
• test/bundler/bundler_edgecase.test.ts
• test/bundler/transpiler/transpiler.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Beyond the inline finding, I also checked whether is_module_ref could over-match a user-shadowed module (it can't — a local let module is not Kind::Unbound), and whether fold_require_main_and_module could double-decrement usage by iterating both operands (it can't — Equality::RequireMainAndModule guarantees exactly one side is an EIdentifier, the other is ERequireMain).

Extended reasoning...

The reported EXPECTED_VERSION finding is valid per the parser header at src/js_parser/parser.rs:1-4 and REVIEW.md's cache-version rule, so this needs a fix before merge. The two other non-mechanical parts of the diff — the widened is_module_ref predicate and the new fold_require_main_and_module loop over both operands — were checked for over-matching and double-ignore_usage respectively; neither is a problem.

[claude]

I reviewed the updated PR and didn't find any bugs — the cache-version bump in c2f6135 addresses my earlier concern. Since this changes how module/exports bind in ESM-by-syntax files with runtime-visible effects (e.g. module.require(x) no longer folds to require(x)), a maintainer look at the intended semantics would still be worthwhile.

What was reviewed:

• is_esm_by_syntax() is shared consistently across prepare_for_visit_pass, is_file_considered_to_have_esm_exports, and the exports_kind selection — the binding decision now agrees with the classification decision.
• is_module_ref's unbound-symbol check only matches kind == Unbound, so a user let module / var module in an ESM file does not spuriously trigger the require.main === module fold.
• fold_require_main_and_module ignores usage of whichever operand is the identifier, so both the CJS module_ref case and the new ESM unbound-module case decrement the right symbol.
• New tests cover the ESM / minified / --format=cjs / define variants plus the require.main === module fold in an ESM file, and the existing IsBuffer1/IsBuffer2 lodash-es guards still hold.

Extended reasoning...

Overview

The PR changes prepare_for_visit_pass in src/js_parser/p.rs so that when a file contains an export keyword or top-level await (the new shared is_esm_by_syntax() predicate), exports_ref/module_ref are created via new_symbol without being placed in the module scope. Bare module/exports in such files then resolve as ordinary unbound identifiers — printed verbatim, reserved by the renamer, and eligible for --define. The require.main === module fold is preserved by extending EqlParser::is_module_ref to also accept an unbound symbol named module, and the four equality arms in visit_binary.rs are deduplicated into fold_require_main_and_module. EXPECTED_VERSION is bumped to 26 (my earlier review flagged this; it was addressed in c2f6135). Tests are added in bundler_edgecase.test.ts and transpiler.test.js.

Security risks

None identified. This is a symbol-binding change in the JS parser; it does not touch auth, crypto, network, filesystem, or untrusted-input parsing beyond what the JS parser already handles.

Level of scrutiny

Medium-high. prepare_for_visit_pass is on the critical path for every parsed file, and the change alters runtime-transpiler output for a class of files (ESM-by-syntax that reference free module/exports). The PR description explicitly enumerates observable behavior changes: module.require(x) is no longer rewritten to require(x) in such files, --define module=... now applies, and the dev server no longer binds these to closure placeholders. These are intentional and match esbuild, but they are a semantic decision a maintainer should sign off on rather than an automated approval.

Other factors

• My previous inline finding (missing cache-version bump) was addressed and the thread resolved.
• The comment-cop bot's requests to shorten explanatory comments were all addressed (914ca8d, 9fdac21, 87aff35), and the long-comment concern was resolved structurally by introducing the shared is_esm_by_syntax() predicate rather than duplicating the condition with a paragraph of justification.
• Test coverage is thorough: four itBundled cases (free identifiers, minified, --format=cjs under both bun and node, define split between ESM and CJS files) plus a Bun.Transpiler test covering export, top-level await, plain CJS, and import-only files. The existing edgecase/IsBuffer1/IsBuffer2 and extra/DefineModule* tests guard the lodash-es dual-mode pattern and the CJS-file define behavior respectively.
• I checked that is_module_ref's new unbound-name match cannot fire on a user-declared module binding (those have a non-Unbound symbol kind), and that fold_require_main_and_module decrements the correct symbol's use count in both the CJS and ESM cases.

[claude]

I reviewed this PR and didn't find any bugs; the earlier finding (missing EXPECTED_VERSION bump) was addressed in c2f6135. Because this changes user-visible parser/bundler semantics — typeof exports in a bundled ESM file goes from "object" to "undefined", module.require(x) is no longer rewritten in ESM-by-syntax files, and files mixing export with module.exports = ... now stay ESM instead of being converted to CJS — a human look at the design decision would still be worthwhile.

Checked: is_esm_by_syntax() is the same predicate the exports_kind selection already used, so binding and classification stay in sync; is_module_ref only matches Unbound symbols named module, so a locally-declared module still blocks the require.main === module fold; the new fold_require_main_and_module helper ignores the actual identifier operand's ref (not p.module_ref), which is correct for the ESM case where they differ; and require_ref is still declared in scope for both cases.

Extended reasoning...

Overview

This PR changes prepare_for_visit_pass in src/js_parser/p.rs so that when a file contains an export keyword or top-level await (the new is_esm_by_syntax() predicate), exports_ref/module_ref are created via new_symbol instead of declare_common_js_symbol — i.e., they exist for internal use (namespace object, HMR, export =) but are not put in scope, so bare module/exports references resolve to ordinary unbound symbols. Supporting changes: the EqlParser trait's module_ref() becomes is_module_ref() so the require.main === module → import.meta.main fold still fires on the unbound identifier; the four equality arms in visit_binary.rs are deduplicated into fold_require_main_and_module, which now ignores the usage of whichever operand is the identifier; parse_entry.rs and is_file_considered_to_have_esm_exports share the same predicate; and EXPECTED_VERSION is bumped to 26. Five new itBundled tests and two Bun.Transpiler tests cover the ESM/CJS/minified/format=cjs/define matrix.

Security risks

None identified. This is a parser scoping/classification change; no untrusted input handling, no new syscalls, no serialization format changes beyond the version bump.

Level of scrutiny

High. This is a deliberate semantic change to bundler and runtime-transpiler output for a class of files, not a mechanical bugfix. The PR description is thorough and the behavior matches esbuild, but the observable changes (dual-mode typeof exports guards now taking the ESM branch in bundled output, module.require no longer rewritten in ESM files, mixed export/module.exports files staying ESM) are design decisions a maintainer should sign off on.

Other factors

My earlier inline finding (runtime transpiler cache version not bumped) was addressed. The comment-cop bot's complaints about long comments were all addressed in follow-up commits. Test coverage is extensive and covers the variant matrix (ESM/TLA, minified, format=cjs run under both bun and node, define, guarded assignments, and the module.require rewrite in both CJS and ESM sources). The is_module_ref change was checked for over-matching: it requires symbol.kind == Unbound, so a user-declared local module (which would be Hoisted/Let/Const/etc.) does not match, and in CJS files module binds to p.module_ref and hits the fast eql path. The fold_require_main_and_module helper correctly ignores the actual identifier ref rather than p.module_ref (which in the ESM case is a different, zero-use symbol).

[robobun]

Superseded by #40840, which takes the same approach (fresh exports_ref/module_ref symbols when the file has export or top-level await, the require.main === module rewrite kept through an unbound-module check) and adds the esbuild warning for CommonJS assignments in such files, the top-level return classification, and a bundle-vs-run test matrix.

[robobun]

Superseded by #40840. Verified against that branch: all seven tests from this PR pass there (edgecase/ESMFileModuleAndExportsAreFreeIdentifiers, edgecase/ESMFileWithGuardedCommonJSAssignmentsStaysESM, edgecase/ESMFileModuleAndExportsAreFreeIdentifiersMinified, edgecase/ESMFileModuleAndExportsAreFreeIdentifiersFormatCJS, edgecase/DefineModuleAndExportsInESMFile, and the two Bun.Transpiler tests). All seven fail on 1.4.1.

#40840 covers this case in cjs/MixedExportAndModuleExportsIsESM_*, cjs/TypeofModuleAndExportsInESM, cjs/RequireMainEqualsModuleInESM, cjs/DefineModuleAppliesOnlyInESMFile, and test/cli/run/esm-cjs-detection.test.ts ("a file with export and module.exports is ESM"). The cases with no counterpart there (module.id and module.require() reads, minified identifiers, --format=cjs under node, the Bun.Transpiler define and module.require() tests) are carried over to #40840 in 533b1b0. Closing in favor of that PR.