Conversation
|
Updated 10:06 AM PT - Jul 18th, 2026
❌ @robobun, your commit 0142918 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33899That installs a local version of the PR into your bun-33899 --bun |
WalkthroughChangesThe runtime transpiler now carries explicit module type through parsing, includes it in cache hashing, rejects ES module syntax in applicable CommonJS contexts, and detects logged parse errors before output generation. Tests cover module-format enforcement and type-aware fixtures. CommonJS module enforcement
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a deliberate breaking change to module-format detection in the core parser (previously-running code now hard-errors), with judgment calls around the TS/bundler exclusions and coordination with #33807 — worth a maintainer's sign-off.
Extended reasoning...
Overview
This PR changes src/js_parser/parse/parse_entry.rs to emit a hard parse error when export or top-level await appears in a file explicitly declared CommonJS (.cjs extension or "type":"commonjs"). It threads module_type through the async transpiler path (RuntimeTranspilerStore.rs, jsc_hooks.rs), adds a post-parse log.errors check on the async path, hashes module_type into the transpiler-cache features hash, bumps the cache version to 23, and updates two test files (16 new tests in run-cjs.test.ts, plus a fixture rewrite in resolve-ts.test.ts to stop relying on the old lenient behavior).
Security risks
None identified. No auth, crypto, network, or untrusted-input parsing surface is touched beyond what the parser already handles; the change only adds an error-emission branch on already-parsed state.
Level of scrutiny
High. This is a behavioral breaking change to core module-loading semantics: code that previously ran under Bun (ESM syntax in .cjs / "type":"commonjs" .js) will now fail with a SyntaxError. While it aligns Bun with Node, the decision to hard-error (vs. warn), the scope of the TypeScript and bundler exclusions, and the ecosystem-impact tradeoff are design decisions a maintainer should own. It also touches the parser hot path and the transpiler cache-key derivation, both of which are critical infrastructure.
Other factors
- The PR is well-reasoned and thoroughly tested (positive cases, negative controls, multiple load paths,
Bun.buildexclusion, TS exclusion). - The
resolve-ts.test.tsfixture change is a real signal that existing code patterns will break — a maintainer should confirm the ecosystem-compat tradeoff is acceptable. - The description notes overlap with #33807 (same
module_typethreading and cache-hash change); landing order needs coordination. - The new
log.errors > 0check on the async transpile path is a secondary behavior change (previously-silent parser errors now surface) that is correct but broadens impact beyond just this feature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/run/run-cjs.test.ts (1)
65-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing "reason" assertion for consistency with sibling tests.
Sibling tests for
.cjs/.jsunder"type":"commonjs"(lines 41-43, 52-53, 57-63) all assert both the generic error message and the specific reason (.cjsextension orpackage.jsontype). This test only asserts the generic top-level-await message, omitting the'the nearest package.json sets "type": "commonjs"'reason check that the sibling export test at line 53 verifies.✏️ Suggested addition
const { stdout, stderr, exitCode } = await run(String(dir), "t.js"); expect(stderr).toContain("Cannot use top-level 'await' in a CommonJS module"); + expect(stderr).toContain('the nearest package.json sets "type": "commonjs"'); expect({ stdout, exitCode }).toEqual({ stdout: "", exitCode: 1 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/run/run-cjs.test.ts` around lines 65 - 73, Add an assertion in the top-level-await CommonJS test to verify stderr also contains the specific reason that the nearest package.json sets "type": "commonjs", matching the sibling tests and retaining the existing generic error assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/cli/run/run-cjs.test.ts`:
- Around line 65-73: Add an assertion in the top-level-await CommonJS test to
verify stderr also contains the specific reason that the nearest package.json
sets "type": "commonjs", matching the sibling tests and retaining the existing
generic error assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd0b60fb-4bf5-4fd0-9de2-569843e4acee
📒 Files selected for processing (3)
test/cli/run/run-cjs.test.tstest/cli/test/bun-test.test.tstest/js/bun/typescript/type-export.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/jsc_hooks.rs`:
- Around line 3944-3945: Update the `RuntimeTranspilerStore::transpile` call
site to seed `ResolvedSourceTag` and `is_commonjs_module` from the same nearest
package that provides `nearest_module_type`, rather than the potentially outer
`package_json.module_type`; alternatively pass the nearest package explicitly.
Add a regression test covering nested nameless packages with dynamic imports and
verify parsing metadata matches the advertised module type.
In `@test/cli/run/run-cjs.test.ts`:
- Around line 143-167: Update the nested ESM test using the existing
`exportBody` and `entry.mjs` setup to export a known value from
`lib/esm/inner/t.js`, capture the namespace returned by the dynamic import, and
assert that value directly in addition to checking stdout and exitCode. Ensure
the assertion validates the resolved ESM module shape rather than only
successful execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60a5185d-c72e-448d-8fdb-b01833de7c56
📒 Files selected for processing (2)
src/runtime/jsc_hooks.rstest/cli/run/run-cjs.test.ts
|
CI status: the new Remaining red across the post-rebase builds is all on tests this diff does not touch: Ready for review. |
There was a problem hiding this comment.
No issues found after the fixes — deferring to a maintainer since this is a user-facing behavior change to core module loading (new hard error where code previously ran, plus the async-path log.errors > 0 check surfaces previously-swallowed parser errors more broadly than just this feature).
Extended reasoning...
Overview
This PR makes .cjs files and .js files under "type":"commonjs" reject ESM export and top-level await with a hard parse error, matching Node's behavior. It touches the JS parser (parse_entry.rs), the runtime transpiler store/cache (RuntimeTranspilerStore.rs, RuntimeTranspilerCache.rs), and the module loader hooks (jsc_hooks.rs), plus four test files. Beyond the headline change, it also (a) rewires module_type derivation to walk to the nearest package.json including nameless ones, and (b) adds a log.errors > 0 check after parse on the async transpile path so parser-emitted errors surface instead of being silently dropped.
Security risks
None identified. No auth, crypto, network, or untrusted-input parsing changes; the new code paths gate on already-parsed AST state and existing resolver metadata.
Level of scrutiny
High. This is a deliberate user-facing breaking change: code that ran on Bun before will now hard-error. The rationale (Node compat) is sound and the PR description is thorough, but whether Bun should be strict here — and the TypeScript carve-out — is a product decision. Separately, the log.errors > 0 check on the async path (RuntimeTranspilerStore.rs:962) has effects beyond this feature: it changes which error message users see for any parser-logged error on the async path, as demonstrated by the type-export.test.ts update. That's the right fix for a real bug (silently executing output despite parse errors), but it widens the blast radius and a maintainer should confirm it's intended.
Other factors
- All four prior review comments (two from me, two from CodeRabbit) were addressed in follow-up commits and are resolved.
- Test coverage is comprehensive (16 new cases covering positive/negative/nested/TS-exclusion/bundler paths).
- The PR notes overlap with #33807 (the ESM-direction sibling); whichever lands second resolves a conflict, so merge order matters.
- The
jsc_hooks.rsnearest-package.json walk changes resolution semantics for dual-package layouts — well-tested here but touches a hot path every module load goes through.
631a33a to
1117ebd
Compare
There was a problem hiding this comment.
No new findings — all four prior review threads are addressed. This is a deliberate breaking change to runtime module classification (code that ran before now hard-errors), so leaving the merge decision to a maintainer.
What was reviewed:
- The new
log.errors > 0check on the async transpile path — confirmed it mirrors the sync path and the collateral behaviour change (Bun's "Multiple exports" parser error now surfacing instead of JSC's) is covered by the updatedtype-export.test.ts. - The nearest-package.json walk in
jsc_hooks.rs— checked that leavingresolved_source.tagkeyed to the enclosing named package (per the resolved CodeRabbit thread) doesn't reintroduce a mismatch, sinceis_commonjs_moduleis recomputed from the parse result. - Cache-hash /
EXPECTED_VERSIONbump covers the newmodule_typeinput. - The import-only-in-explicit-CJS gap is now documented at the fallthrough branch.
Extended reasoning...
Overview
Makes export and top-level await in .cjs files (and .js under "type":"commonjs") a hard parse error at runtime, matching Node. Touches parse_entry.rs (the error itself), RuntimeTranspilerStore.rs (thread module_type to the worker + check log errors post-parse), jsc_hooks.rs (derive module_type from the nearest package.json including nameless ones), RuntimeTranspilerCache.rs (hash module_type, bump version to 24), and four test files.
Security risks
None. No untrusted-input parsing surface is widened; the change narrows accepted syntax.
Level of scrutiny
High — this is an intentional user-visible breaking change to runtime behaviour, and the author explicitly noted it's awaiting product sign-off. The log.errors > 0 check on the async path is also a broader behaviour change than the headline (any parser-emitted log error on the async path now surfaces where it previously didn't), which is correct but worth a maintainer's eye. Not appropriate for bot approval.
Other factors
I've reviewed this across four passes; all inline comments (test stderr shape, .cjs remedy-note wording, import-only exclusion) were applied. The 18-test matrix in run-cjs.test.ts covers the variant space well (direct/imported/dynamic/require, both nested-nameless directions, TS carve-out, bundler exclusion). The bug hunter found nothing this run. The overlap with #33807 (same module_type threading / cache-hash change) is called out in the description.
There was a problem hiding this comment.
No bugs found, but deferring to a human: this is a deliberate breaking change to runtime module classification (code that ran under Bun will now hard-error), and the author noted it's awaiting product sign-off.
What was reviewed:
- Parser guard in
parse_entry.rs— gated on!bundle && !ts && commonjs_at_runtime; bundler/TS/JSX paths unaffected. RuntimeTranspilerStorelog-errors check placement — runs after the watcheradd_file, matching the parse-Nonebranch, so hot-reload isn't skipped on the new error.jsc_hooksnearest-package.json walk —pkg_name/ResolvedSourceTagsemantics unchanged; only the parser'smodule_typeinput widens.- Cache:
module_typehashed +EXPECTED_VERSIONbumped, so no stale-hit path reinstates the old behaviour.
Extended reasoning...
Overview
The PR makes export and top-level await in an explicitly-CommonJS file (.cjs, or .js under "type":"commonjs") a hard runtime parse error, matching Node. It threads an extension-and-nearest-package.json-aware module_type from jsc_hooks through RuntimeTranspilerStore::transpile into the parser, adds a post-parse log.errors > 0 check on the async transpile path (previously only the sync path had it), hashes module_type into the transpiler cache key, and bumps the cache version. Three existing test files are adjusted to stop relying on the old lenient behaviour, and run-cjs.test.ts gains 17 new tests covering the error, negative controls, TS exclusion, nested nameless-package.json layouts, and the bundler carve-out.
Security risks
None identified. No untrusted input parsing changes shape; the new code only reads already-parsed module_type/extension state and emits a diagnostic. No auth, crypto, network, or filesystem-write surface is touched.
Level of scrutiny
High. This is core runtime behaviour in the parser and module loader — every .js/.cjs file loaded at runtime flows through the changed decision. It is an intentional breaking change: files that Bun previously ran will now fail with a SyntaxError. That is a product/compat call (how strictly to match Node vs. tolerate mixed-syntax packages in the wild) that a maintainer should sign off on. The author explicitly flagged it as "a deliberate breaking change awaiting product sign-off."
Other factors
- All four of my prior inline comments were addressed (stderr-assertion shape,
.cjsremedy-note wording, import-only exclusion rationale, and — via the CodeRabbit thread — nested-ESM namespace assertions). The latest commits since my last comment are just the documenting comment (23d485c) and a CI retrigger. - The
type-export.test.tschange is a real behaviour change (async path now surfaces Bun's parser error instead of JSC's), not just a fixture tweak — worth a maintainer glance to confirm that's the desired outcome. - The sibling PR #33807 (ESM direction) shares the
jsc_hooks/RuntimeTranspilerStore/cache-hash changes; whichever lands second has a small merge conflict to resolve. - Test coverage is thorough (direct/static-import/dynamic-import/require entry paths, both nested-nameless-package directions, TS/bundler exclusions), and CI on the last full build was green for all touched suites per the author's build-71435 summary.
63007bb to
5168d62
Compare
There was a problem hiding this comment.
No bugs found, but this is a deliberate breaking change (previously-working .cjs/"type":"commonjs" files with export/TLA now hard-error) that the author notes is awaiting product sign-off — deferring to a maintainer for that call.
What was reviewed:
- Parser gate in
parse_entry.rs: confirmed it's runtime-only (!bundle,commonjs_at_runtime), TS-excluded, and the.cjsremedy note no longer suggests the ineffective package.json fix. RuntimeTranspilerStoreasync path: the newlog().errors > 0check runs after the watcheradd_fileblock (matching the sync path's ordering), andmodule_typeis now threaded from the caller instead of re-derived fromthis_tag.jsc_hooks.rsnearest-package.json walk: leavespackage_json/ResolvedSourceTagsemantics unchanged (only the newnearest_module_typefield walks nameless parents), soignoreESModuleAnnotationbehaviour is preserved.- Cache:
module_typeis hashed andEXPECTED_VERSIONbumped, so byte-identical sources under different module types don't collide.
Extended reasoning...
Overview
The PR changes runtime module-type enforcement so that .cjs files and .js files under an explicit "type":"commonjs" package.json reject ESM export and top-level await with a parser error, matching Node's SyntaxError behaviour. It touches the parser's exports_kind decision (parse_entry.rs), threads module_type through the async transpile worker (RuntimeTranspilerStore.rs), derives module_type from the nearest package.json including nameless ones (jsc_hooks.rs), hashes module_type into the transpiler cache key with a version bump (RuntimeTranspilerCache.rs), and updates four test files.
Security risks
None identified. This is module-format classification and error reporting; no auth, crypto, untrusted-input parsing, or filesystem-escape surface is touched. The new DirInfo parent walk reuses the existing resolver cache.
Level of scrutiny
High. This is an intentional breaking change to core runtime module-loading semantics: code that ran on every prior Bun release will now fail. The author explicitly flagged it as "a deliberate breaking change awaiting product sign-off". That is precisely the kind of decision a maintainer must own — the correctness of the implementation (which looks sound and is well-tested across 18 new cases including negative controls, TS carve-outs, dual-package layouts, and the bundler exclusion) is separate from the product decision to ship it. The sibling PR #33807 handles the ESM direction of the same decision-order bug and shares plumbing, so whichever lands second will need a small merge resolution.
Other factors
All three of my prior inline comments (stderr-assertion shape, .cjs remedy-note wording, and the import-only exclusion) were addressed in follow-up commits, as were both CodeRabbit findings. The async-path log.errors check is a nice incidental fix (it's why the type-export.test.ts expectation changed). CI on build 75048 was green for the touched tests; the three unrelated failures noted there (fs-promises FileHandle, macOS RSS threshold, perl-daemon timeout) are pre-existing on main. Given the scope and the author's own note that this needs maintainer sign-off, approving without human review isn't appropriate here.
5168d62 to
8ac57f0
Compare
There was a problem hiding this comment.
No further issues from my side — the three earlier inline comments are all addressed. Deferring because this is a deliberate breaking change to runtime module classification (code that previously ran now hard-errors), which as you noted is awaiting product sign-off from a maintainer.
What was reviewed:
parse_entry.rsguard: confirmed it's gated on!bundle && !ts && commonjs_at_runtime, so bundler and TS paths are untouched;.cjsvs package.json branching in the diagnostic is correct.RuntimeTranspilerStorelog.errors > 0check matches the sync path intranspile_source_code_inner; thetype-export.test.tschange is a correct consequence, not a weakened assertion.jsc_hooks.rsnearest-package.json walk:package_jsonreturn value is unchanged (stillenclosing_package_json), soresolved_source.tag/ignoreESModuleAnnotationsemantics are preserved; only the parser'smodule_typeinput widens.- Cache:
module_typehashed + version bump to 24, so no stale-hit risk.
Extended reasoning...
Overview
Touches the runtime module-format decision across four layers: parse_entry.rs (emit a range error when export/TLA appears under explicit ModuleType::Cjs), jsc_hooks.rs (derive module_type from the nearest package.json including nameless ones, and thread it to the async transpile call), RuntimeTranspilerStore.rs (carry module_type on TranspilerJob instead of re-deriving from resolved_source.tag; check log.errors after a successful parse so parser-emitted errors surface on the async path), and RuntimeTranspilerCache.rs (hash module_type + bump EXPECTED_VERSION). Four test files updated: 18 new cases in run-cjs.test.ts, plus fixture rewrites in resolve-ts, bun-test, and type-export that were relying on the old lenient behaviour.
Security risks
None. No untrusted-input parsing, no auth/crypto/permissions. The change tightens what runs rather than loosening it.
Level of scrutiny
High — this is the core module-loading hot path executed for every file, and it is an intentional user-facing breaking change: .cjs files and .js under "type":"commonjs" that contain export or top-level await used to run and will now error. The PR author explicitly notes it is "a deliberate breaking change awaiting product sign-off". That is a design/compat call a maintainer needs to make; the sibling PR #33807 (the ESM direction) is in the same boat and whichever lands second has a small merge conflict to resolve. The secondary behaviour change — the async transpile path now surfaces parser log errors instead of silently executing the output — is also a strict tightening that could surface pre-existing errors in user code.
Other factors
All three of my earlier inline comments (stderr-empty assertions, .cjs remedy-note wording, and the import-only exclusion) have been addressed in follow-up commits. CodeRabbit's two comments were also addressed/withdrawn. Test coverage is thorough (error cases × entry paths, negative controls, TS exclusion, nested nameless-package.json both directions, bundler exclusion). The implementation looks correct and internally consistent to me; deferral is purely for the product decision on the breaking change, not for a code concern.
A .js file under "type":"commonjs" (or a .cjs file) containing `export` or top-level `await` was silently classified and run as ESM. In Node those are a hard SyntaxError: an explicit "type":"commonjs" pins the format of every .js in the tree, and .cjs is always CommonJS. The exports_kind decision in parse_entry.rs checked `esm_export_keyword || top_level_await_keyword` before ever consulting `options.module_type`, so the syntax out-ranked the explicit declaration. - parse_entry.rs: when `module_type == Cjs` at runtime (not bundling, not TypeScript), emit a range error pointing at the `export`/`await` with a note saying why the file is CommonJS. TypeScript is excluded because `export` in a CommonJS-typed .ts/.cts is idiomatic (tsc compiles it to `exports.x = ...`). - RuntimeTranspilerStore.rs: thread the caller's extension-aware `module_type` onto `TranspilerJob` and use it in the worker. The worker was re-deriving it from `resolved_source.tag`, which never carried the .cjs/.mjs extension signal. Also check `log.errors` after a successful parse (matching the sync path in transpile_source_code_inner); without this, parser-emitted errors on the async path were silently dropped and the output executed anyway. - jsc_hooks.rs: pass the computed `module_type` to `transpiler_store.transpile()`. - parse_entry.rs/RuntimeTranspilerCache.rs: hash `module_type` into the runtime transpiler features hash and bump EXPECTED_VERSION to 23, since byte-identical sources now parse differently under different module types. - resolve-ts.test.ts: the `type:commonjs && jsFile` matrix rows wrote ESM `export` into a .js under "type":"commonjs" and relied on the old lenient behaviour; write CJS-shaped sources for that combination. Sibling of #33807, which makes "type":"module" authoritative over bare `module`/`exports` references (the ESM direction of the same decision-order bug). The RuntimeTranspilerStore module_type threading and cache-hash change here are the same as in that PR; whichever lands second resolves a small conflict.
- bun-test.test.ts: the "cjs dynamic import" fixture used top-level `await` in a .cjs file, which has never been valid CommonJS. Rewrite it to use `.then()` so the fixture is actually CJS and still exercises dynamic `import()`. - type-export.test.ts: the "through export merge" main.* cases expected JSC's "Cannot export a duplicate name" SyntaxError. That only happened because the async transpile path silently dropped Bun's own "Multiple exports with the same name" parser error and handed the broken output to JSC. With the log-errors check, Bun's parser error now surfaces on both the direct and imported paths.
Match the shape used by the other tests in the block so benign debug/ASAN stderr noise can't produce a false failure.
…g nameless ones
`enclosing_package_json` skips package.json files that have no "name"
field, so for dual-package layouts like puppeteer (root
`{"type":"commonjs"}` with a nameless `lib/esm/package.json`
`{"type":"module"}`) the module_type for `lib/esm/**/*.js` was
resolved from the outer package.json instead of the nearer one, and the
new ESM-in-CommonJS error fired on valid ESM files.
Walk DirInfo parents until a package.json is found (regardless of name)
and use that for the .js/.ts module_type. Carry it as a separate
LoaderResult field so `pkg_name` semantics are unchanged.
Two tests for the nested-override in each direction.
The .cjs extension overrides package.json's "type" field, so that half of the remedy note was ineffective. Branch on the extension the same way the 'why' note does.
Prove the named export is reachable and no synthetic default is attached, not just that execution succeeds.
Node rejects a static import statement in a .cjs/"type":"commonjs" .js the same way it rejects export/TLA, but that case is deliberately left alone here: import-only in explicit-CJS is far more prevalent in the wild than export/TLA, and rejecting it would be much more breaking.
8ac57f0 to
0142918
Compare
There was a problem hiding this comment.
No further findings — the earlier nits (stderr assertions, .cjs remedy note, import-only exclusion) are all addressed. Deferring to a maintainer since this is a deliberate breaking change to module-format classification and, per the thread, is awaiting product sign-off.
What was reviewed:
- The new
log.errors > 0check on the async transpile path — confirmed it mirrors the sync path and thetype-export.test.tsupdate reflects a real behaviour change (previously-swallowed parser errors now surface). nearest_module_typewalk injsc_hooks.rs— checked it only feeds the parser'smodule_typeand leavesresolved_source.tagsemantics unchanged (per the resolved CodeRabbit thread).- Cache-key change —
module_typeis hashed andEXPECTED_VERSIONbumped, so stale entries can't bypass the new error. - Test fixture rewrites in
resolve-ts.test.ts/bun-test.test.ts— verified they preserve what the original tests asserted.
Extended reasoning...
Overview
This PR changes Bun's runtime module-format classification so that .cjs files and .js files under "type":"commonjs" reject ESM export and top-level await with a parse error, matching Node's SyntaxError behaviour. It touches four native files (parse_entry.rs, RuntimeTranspilerCache.rs, RuntimeTranspilerStore.rs, jsc_hooks.rs) and four test files. The infrastructure changes — threading module_type through TranspilerJob, walking to the nearest package.json (including nameless ones) for the runtime module_type, hashing module_type into the transpiler cache key, and checking log.errors after a successful async parse — are shared with #33807 and have broader effect than the headline feature.
Security risks
None identified. No untrusted input parsing beyond what the parser already does; no auth/crypto/permissions surface.
Level of scrutiny
High. This is an intentional breaking change to core runtime module-loading semantics: code that ran on every prior Bun release will now hard-error. The log.errors > 0 check on the async transpile path is independently significant — it changes which parser diagnostics surface at runtime for any module transpiled off-thread, not just the new CJS-rejects-export case (the type-export.test.ts update demonstrates this). The nearest_module_type derivation in jsc_hooks.rs also subtly changes what module_type the parser sees for .js/.ts under nameless nested package.json layouts. These are the right changes, well-tested (18 new tests covering the matrix, plus negative controls and the dual-package layout), and CI is green on the touched suites — but the product decision of whether/when to ship the breaking change is a maintainer call, and the author's own comment says it is "awaiting product sign-off".
Other factors
- All three of my prior inline nits and both CodeRabbit findings are resolved with follow-up commits.
- The PR description explicitly flags a merge-order conflict with #33807 (same infrastructure changes); a human should coordinate landing order.
- The
import-only-in-explicit-CJS case is deliberately left out and now documented at the branch site, per the resolved thread. - Test coverage is thorough: direct/static-import/dynamic-import/require entry points,
.cjsvs"type":"commonjs", extension-wins, nameless-nested-package both directions, TypeScript exclusion, andBun.buildunaffected.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Problem
A
.jsfile under"type":"commonjs"(or a.cjsfile) containingexportor top-levelawaitis silently classified and run as ESM. In Node those are a hard SyntaxError: an explicit"type":"commonjs"pins the format of every.jsin the tree, and.cjsis always CommonJS. A program that cannot load on any Node loads and runs on Bun, and nothing warns.bun pkg/t.jsranimport {x} fromnamed 7Cause
In
parse_entry.rstheexports_kinddecision checksesm_export_keyword || top_level_await_keywordbefore ever consultingoptions.module_type, so the syntax out-ranks the explicit declaration.Fix
parse_entry.rs: whenmodule_type == Cjsat runtime (not bundling, not TypeScript), emit a range error at theexport/awaitwith a note saying why the file is CommonJS. TypeScript is excluded becauseexportin a CommonJS-typed.ts/.ctsis idiomatic (tsc compiles it toexports.x = ...).RuntimeTranspilerStore.rs: thread the caller's extension-awaremodule_typeontoTranspilerJoband use it in the worker. The worker was re-deriving it fromresolved_source.tag, which never carried the.cjs/.mjsextension signal. Also checklog.errorsafter a successful parse (matching the sync path intranspile_source_code_inner); without this, parser-emitted errors on the async path were silently dropped and the output executed anyway.jsc_hooks.rs: derive the runtimemodule_typefor.js/.tsfrom the nearest package.json'''s"type"by walkingDirInfoparents, including nameless ones thatenclosing_package_jsonskips. Without this, dual-package layouts like puppeteer (root{"type":"commonjs"}, namelesslib/esm/package.json={"type":"module"}) were seeing the outer type and the new error fired on valid ESM files. Carried as a separateLoaderResultfield sopkg_namesemantics are unchanged. Also pass the computedmodule_typetotranspiler_store.transpile().module_typeinto the runtime transpiler features hash and bumpEXPECTED_VERSIONto 23, since byte-identical sources now parse differently under different module types.resolve-ts.test.ts: thetype:commonjs && jsFilematrix rows wrote ESMexportinto a.jsunder"type":"commonjs"and relied on the old lenient behaviour; write CJS-shaped sources for that combination (the resolution behaviour under test is the same either way).bun-test.test.ts: the "cjs dynamic import" fixture used top-levelawaitin a.cjsfile; rewrite it to use.then()so the fixture is actually CommonJS.type-export.test.ts: the "through export merge"main.*cases expected JSC'sCannot export a duplicate nameerror, which only surfaced because the async transpile path was silently dropping Bun's ownMultiple exports with the same nameparser error. With the log-errors check, Bun's parser error now surfaces on both the direct and imported paths.Relationship to #33807
#33807 makes
.mjs/"type":"module"authoritative over baremodule/exportsreferences (the ESM direction of the same decision-order bug). This PR is the CommonJS direction. TheRuntimeTranspilerStoremodule_typethreading, thejsc_hooksnearest-package.json walk, and the cache-hash change here are the same as in #33807; whichever lands second resolves a small conflict.Tests
test/cli/run/run-cjs.test.tsgains adescribeblock covering.cjs,"type":"commonjs".js, top-levelawait, loaded directly / via static import from.mjs/ via dynamicimport()/ viarequire(), plus negative controls (.mjs,"type":"module", typeless.js,.mjsinside"type":"commonjs",.cts/.tsexcluded,Bun.buildunaffected). 7 of the 16 new tests fail on the unfixed build and all pass with this change.Also ran:
test/js/bun/resolve/,test/bundler/bundler_cjs2esm.test.ts,test/bundler/transpiler/transpiler.test.js,test/cli/test/isolation.test.ts,test/integration/jsdom/with no new failures.no test proof · iteration 13 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts test/js/bun/typescript/type-export.test.ts