bundler: fix re-exports of the "bun" builtin with --target=bun - #37829
bundler: fix re-exports of the "bun" builtin with --target=bun#37829robobun wants to merge 7 commits into
Conversation
… from non-entry export stars
With --target=bun, a module doing `export * from "bun"` or
`export { default } from "bun"` produced undefined bindings, and with
--format=cjs the re-export also copied the whole Bun object onto the
entry point's module.exports.
- convertStmtsForChunk: a runtime `export * from "bun"` in ESM output
was turned into `import * as ns from "bun"` plus `__reExport(exports,
ns)`. The printer lowers that import to `var ns = globalThis.Bun`,
which is not hoisted, so __reExport ran before ns was assigned. The
"bun" record now passes the module expression (printed as
globalThis.Bun) to __reExport directly, as the CJS path already did.
- convertStmtsForChunk: the module.exports argument of __reExport was
added for every file in an entry chunk instead of only the entry
point file, so any non-entry `export * from <external>` leaked onto
the entry's module.exports. For "bun" that copied fetch/serve there,
which made `bun out.cjs` start a server. importstar/
ReExportStarEntryPointAndInnerFileExternal had the leaked key baked
into its expected output; the non-external variant of the same test
already expected the key to be absent.
- js_printer: `import { default as x } from "bun"` (which is also what
the bundler turns `export { default } from "bun"` into) was printed as
`var { default: x } = globalThis.Bun`. The default export of "bun" is
the Bun object itself, so such items now bind to the module value.
- js_printer: in CJS output the linker marks default/namespace imports of
"bun" with WRAP_WITH_TO_ESM and emits the __toESM helper, but the
globalThis.Bun shortcut ignored the flag, so `import x from "bun"`
became `import_bun.default` on the bare Bun object. The flag is now
honored, giving the import the same namespace shape the module loader
produces for "bun".
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
|
Status: reproduced on 1.4.0 and main with the commands in the description (ESM: CI (build 93270, finished): 178 jobs passed. The one failed job (darwin 14 aarch64) is two |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes bundler output-shape logic in the linker (the per-file vs per-chunk module_exports_for_export gating affects all CJS bundles with a non-entry export * from <external>, and updates an existing esbuild-ported test's expected output), a maintainer look would still be worthwhile.
What was reviewed:
is_entry_point()is exactly!= Kind::None, soshould_strip_exportsis behavior-preserving; onlymodule_exports_for_exportchanges.- The updated
ReExportStarEntryPointAndInnerFileExternalexpectation matches its non-external twin and aligns with whereforce_include_exports_for_entry_pointis set (per-file, LinkerContext.rs:526). print_internal_bun_import_valuereorders but preserves the original three-branch precedence; the manualicounter correctly skips leading commas whendefaultitems are filtered.WRAP_WITH_TO_ESMhandling inprint_require_or_import_exprdoesn't affectrequire("bun")or named-only imports (flag isn't set on those records).
Extended reasoning...
Overview
The PR fixes four bundler bugs around re-exporting the "bun" builtin, two of which are not "bun"-specific. Changes touch convertStmtsForChunk.rs (linker chunk conversion), generateCodeForFileInChunkJS.rs (mechanical parameter drop), and js_printer/lib.rs (print_internal_bun_import refactor + WRAP_WITH_TO_ESM handling). Five test files add coverage; one existing esbuild-ported test's expected stdout is updated.
Security risks
None. This is bundler output-shaping logic; no untrusted-input parsing, no auth/crypto, no filesystem or network paths.
Level of scrutiny
High. The bundler linker is a critical hot path, and one of the four fixes (keying module_exports_for_export off the file's entry-point kind rather than the chunk's) changes CJS output for every non-entry export * from <external>, not just "bun". That change updates an existing test expectation (importstar/ReExportStarEntryPointAndInnerFileExternal). The reasoning is sound — force_include_exports_for_entry_point is set per-file at LinkerContext.rs:526, and the non-external sibling test already expected the corrected shape — but a maintainer should confirm the behavior change is intended.
Other factors
- I verified
EntryPoint::Kind::is_entry_point()is defined asself != Self::None(LinkerGraph.rs:53-55), so theshould_strip_exportsrefactor is behavior-preserving. - The
print_internal_bun_importrefactor extracts three identical match blocks intoprint_internal_bun_moduleand reorders (but does not change) the default-name/star-name/module-expression precedence intoprint_internal_bun_import_value. - Test coverage is thorough: ESM and CJS output, minification (semicolon-insertion sibling), runtime transpiler, and the generic-external case for the
module.exportsleak. - No prior reviews on this PR.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 1:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 336d975 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37829That installs a local version of the PR into your bun-37829 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes bundler linker/printer codegen in a way that affects all non-entry export * from <external> in CJS output (not just "bun") and updates an existing test's expected output, a human look would still be worthwhile.
What was reviewed:
should_strip_exportssemantics unchanged —is_entry_point()is!= Kind::None, same as before.print_internal_bun_import_valueordering (default → star → module) matches the old destructuring-RHS logic for all combinations.WRAP_WITH_TO_ESMpath usesself.options.to_esm_ref, which is populated from the runtime scope inpostProcessJSChunk.- The updated
ReExportStarEntryPointAndInnerFileExternalexpectation now matches its non-external twin.
Extended reasoning...
Overview
Four fixes across the bundler linker and JS printer for how the "bun" builtin is emitted when re-exported or default-imported, plus one general fix: convertStmtsForChunk now keys the module.exports third argument of __reExport(...) on the file's entry-point kind rather than the chunk's, so a non-entry export * from <external> in a CJS bundle no longer leaks onto the entry's module.exports. The now-unused chunk parameter is dropped from both call sites in generateCodeForFileInChunkJS. print_internal_bun_import is refactored to emit default-aliased clause items as var x = <module> and skip them in the destructuring pattern; print_require_or_import_expr honors WRAP_WITH_TO_ESM on the "bun" record. Five test files add coverage; one existing esbuild-ported test's expected stdout is updated.
Security risks
None identified. No user-input parsing, no path/security checks touched — this is bundler AST-to-code emission.
Level of scrutiny
Medium-high. The bundler linker (convertStmtsForChunk) and printer are on the hot path for every bun build, and the module_exports_for_export change is a general semantic change (all externals in CJS output), not scoped to the "bun" builtin. The updated importstar/ReExportStarEntryPointAndInnerFileExternal expectation is well-justified (its non-external twin already expected the corrected shape, and the PR description explains the old expectation encoded the bug), but changing an existing test's expected output warrants human confirmation that the new behavior is what maintainers want.
Other factors
- The comment-cop bot threads on
lib.rs/convertStmtsForChunk.rsappear to have been addressed in commits 41206bc and 5c8d427; the remaining comments in the diff are all single-line. - Test coverage is thorough: ESM and CJS output, entry and non-entry files, default alias with and without other named items, minified output for semicolon insertion, and the runtime transpiler path.
- I verified
is_entry_point()onEntryPoint::Kindisself != Self::None(LinkerGraph.rs:53-55), soshould_strip_exportsis semantically unchanged despite the refactor.
There was a problem hiding this comment.
Beyond the inline nit, I checked that Kind::is_entry_point() is exactly != Kind::None (LinkerGraph.rs:53-54), so should_strip_exports is unchanged — only module_exports_for_export moved from chunk-level to file-level. The updated ReExportStarEntryPointAndInnerFileExternal expectation now matches its non-external twin at importstar.test.ts:1409-1433, which already omitted b. That behavior change (problem 4) affects all CJS bundles with non-entry export * from <external>, so a maintainer sign-off is still worth having.
Extended reasoning...
The four fixes and their tests read correctly; the one non-"bun"-specific semantic change (keying module_exports_for_export off the file rather than the chunk) is well-argued and consistent with the sibling non-external test, but it changes general bundler output and edits an esbuild-ported expectation, so I'm deferring rather than approving. The dynamic-import arm noted inline is pre-existing and correctly scoped out.
The printer now lowers `import { default as x } from "bun"` differently,
and cached output written by an older binary would otherwise be reused
as-is after an upgrade (a release build restoring such an entry still
binds x to undefined).
There was a problem hiding this comment.
I re-reviewed after the description update and comment trims — no bugs found, and the earlier note about the ImportKind::Dynamic arm is now covered in the description. Given this changes general CJS linker semantics (the module_exports_for_export gating moves from chunk-level to file-level, which updates an esbuild-ported test's expected output) alongside three "bun"-specific printer fixes, a human sign-off is still worthwhile.
What was reviewed:
should_strip_exportsis unchanged —Kind::is_entry_point()is exactly!= Kind::None(LinkerGraph.rs:53), so onlymodule_exports_for_exportgating moved.print_internal_bun_import_valuepreserves the old RHS-selection order for the destructuring pattern; the new per-default-itemvaremission callsprint_semicolon_if_needed()first and reuses the default/namespace symbol when one exists.- The
is_bun_builtinbranch drops the statement viacontinue 'stmt_loopso no deadS::ExportStarreaches the wrapper-prefix append; the non-bun path is byte-identical to before. - Cache-version bump matches the runtime-transpiler output change for
import { default as x } from "bun".
Extended reasoning...
Overview
Four related fixes to how the bundler and runtime transpiler handle the "bun" builtin (and one general CJS linker fix that surfaced through it):
convertStmtsForChunk.rs: for a runtimeexport * from "bun"in a non-entry file with ESM output, emit__reExport(exports_re, globalThis.Bun)directly via anE::RequireStringargument instead of animport * as nsstatement (which the printer lowers to an unhoistedvar, ordering it after the__reExportcall).convertStmtsForChunk.rs: gate themodule.exportsthird argument to__reExporton the file's entry-point kind rather than the chunk's, so a non-entry file'sexport * from <external>in a CJS bundle no longer copies the external's keys onto the entry'smodule.exports. This is the general (non-"bun") fix and is what changesimportstar/ReExportStarEntryPointAndInnerFileExternal's expected output. The now-unusedchunkparameter is dropped from both call sites ingenerateCodeForFileInChunkJS.rs.js_printer/lib.rsprint_internal_bun_import: clause items aliaseddefaultare emitted asvar x = <module>and skipped in the destructuring pattern; the three copies of the module-expression printing are folded intoprint_internal_bun_module/print_internal_bun_import_value.js_printer/lib.rsprint_require_or_import_expr: honorWRAP_WITH_TO_ESMon the"bun"record so CJS-format output emits__toESM(globalThis.Bun)and.defaultresolves (fixes #20670 for--bytecode).RuntimeTranspilerCache.rs: version bump 25→26 because the runtime transpiler's output forimport { default as x } from "bun"changed.
New tests in bundler_bun.test.ts, bundler_cjs.test.ts, bundler_minify.test.ts, and import-meta.test.js cover each fix; importstar.test.ts updates one expected output.
Security risks
None identified. This is bundler/printer output-shape work; no untrusted input parsing, no auth/crypto/permissions, no filesystem or network paths touched beyond the pre-existing cache-version constant.
Level of scrutiny
Medium-high. The "bun"-specific printer changes are narrow and well-tested, but fix (2) is a general CJS linker semantics change that affects any bundle with a non-entry export * from <external>, and it revises an esbuild-ported test's expected output. I verified Kind::is_entry_point() is exactly self != Kind::None, so should_strip_exports is unchanged and only the module_exports_for_export gate moved. The non-external twin test already expected the new shape, and the new cjs/__reExport_external_in_non_entry_file test pins it directly, so the change is well-argued — but a maintainer should confirm the intent for the esbuild-ported case.
Other factors
- My earlier inline note about the untouched
ImportKind::Dynamicarm was addressed: the description now explicitly scopes it out to #37730, andimport-meta.test.js:228currently pins the old behavior. - All comment-cop flags are resolved (comments trimmed to one line each; the cache-version note follows the existing changelog block format).
- Test coverage is thorough across ESM/CJS × entry/non-entry × default/named/star, plus the minify variant for semicolon insertion and the runtime-transpiler path.
- No memory-safety concerns: the new code allocates via the existing
Expr::init/Stmt::allocarena helpers and theis_bun_builtinbranch cleanlycontinues the statement loop.
Deferring rather than approving because this is four fixes across the linker and printer, one of which changes general (non-"bun") CJS output and an existing test expectation.
2 participants
Problem
--target=bun, a non-entry file containingexport * from "bun"re-exports nothing:Globimported through it isundefined. In--format=cjsoutput the same input also copies the whole Bun object onto the entry'smodule.exports, so running the bundle printsStarted development server: http://localhost:3000and never exits. No build diagnostic in either case; reproduces on 1.4.0 and main.export { default } from "bun", a default import of"bun"in cjs output (what--bytecodeusers hit), andimport { default as x } from "bun"in both the bundler and the runtime transpiler bindundefinedinstead ofBun: each was lowered to a read ofBun.default, which does not exist."bun"prints as a plainvar, not a hoisted import, and the__reExport()call landed before it.module.exportswas keyed on the chunk being an entry point, not the file, so any non-entryexport * from <external>in a cjs bundle leaked onto the entry's exports, plain externals included.Fixes #20670
Fix
export * from "bun"now passesglobalThis.Bunto__reExport()directly and emits no import, so there is nothing left to hoist. The entry-point form is printed verbatim as before.module.exports, the same condition that emits that file'smodule.exports = ...line. One existing importstar expectation encoded the leak and is updated.defaultbind to the module object itself, and a"bun"import the linker marked for__toESMwrapping prints as__toESM(globalThis.Bun), which does have.default;require("bun")and named imports still print bareglobalThis.Bun. The runtime transpiler cache version is bumped so files cached with the old lowering are not reused.(await import("bun")).defaultin bundles is stillundefined; Stop rewriting a literal import("bun") to Promise.resolve(globalThis.Bun) #37730 removes that rewrite, and bundler: emit namespace binding for runtimeexport * from <external>#36714 (the generic ESMexport *external bug) is untouched.default as x; the repro was also run by hand under--compile --bytecodein both formats.Background
--target=bun,"bun"is a builtin: neither the bundler nor the runtime transpiler loads a module for it, they printglobalThis.Bunin its place, soimport { Glob } from "bun"becomesvar { Glob } = globalThis.Bun;. Avarruns where it sits; a real import is hoisted."bun"module's default export is the Bun object itself; there is noBun.defaultproperty.__reExport(exports, mod, module.exports?)is the runtime helper behindexport * froman external: it copiesmod's properties ontoexports, and ontomodule.exportstoo when the third argument is passed. The third argument is only meant for the entry point of a cjs bundle.__toESM(mod)wraps a CommonJS-shaped value as an ESM namespace, adding adefaultthat points at the value. The linker flags an import record when references to it must go through this wrapper, which is what a default import in cjs output does.Original description
Repro
Reproduces on 1.4.0 and on main. No build diagnostic in any of these cases.
Cause
Four separate problems, two of them not specific to
"bun":export * from "bun"in a non-entry file, ESM output (convertStmtsForChunk): the export star is turned intoimport * as bun from "bun"plus a__reExport(exports_re, bun)prefix. For a real external that works because imports are hoisted, but the printer lowers an import of"bun"to a plainvar bun = globalThis.Bun, which ends up after the call:export { default } from "bun"(bundler, ESM output) andimport { default as x } from "bun"(bundler and runtime transpiler) reachprint_internal_bun_importas a clause item aliaseddefault, which it printed asvar {default: default2 } = globalThis.Bun;. The default export of the"bun"module is the Bun object itself, andBun.defaultdoes not exist.import x from "bun"/export { default } from "bun"with--format=cjs(which--bytecodedefaults to; this is Bun import fails (undefined) when using the bytecode flag #20670): the linker converts the import tovar import_bun = require("bun"), references becomeimport_bun.default, and it setsWRAP_WITH_TO_ESMon the record (the__toESMhelper is already present in such bundles). TheglobalThis.Bunshortcut inprint_require_or_import_exprignored the flag, so the output wasvar import_bun = globalThis.Bun;and.defaultwas undefined.module.exportsthird argument of__reExport(exports, mod, module.exports)was added when the chunk is an entry point rather than when the file being converted is the entry point (convertStmtsForChunkline 72), so every non-entryexport * from <external>in a CJS bundle was copied onto the entry'smodule.exports. With"bun"that copiesfetch/servethere, and running the bundle then starts a server:re.mjs: export * from "ext"maderequire("./out.cjs")return ext's exports even though the entry exports nothing.(The generic ESM variant of 1, where a non-entry
export * from <external>loses its* as nsbinding entirely, is #36714 and is not touched here; the two changes are independent.)Fix
convertStmtsForChunk: for a runtime export star whose record is the"bun"builtin, pass the module expression (anE::RequireString, which the printer emits asglobalThis.Bun) to__reExportdirectly and drop the statement, which is what the CJS output path already produced. Output is now__reExport(exports_re, globalThis.Bun);. The entry point case, whereexport * from "bun"is printed verbatim, is unchanged.convertStmtsForChunk: keymodule_exports_for_exportoff the file's own entry point kind (the same condition that emits the file'smodule.exports = __toCommonJS(...)), and drop the now unusedchunkparameter.print_internal_bun_import: clause items aliaseddefaultare emitted asvar x = <module>;and left out of the destructuring pattern; the three copies of the module expression printing are folded into a helper.print_require_or_import_expr: honorWRAP_WITH_TO_ESMfor the"bun"record, givingvar import_bun = __toESM(globalThis.Bun);. This also applies toimport * as ns from "bun"in CJS output, where the linker sets the same flag:nsbecomes the__toESMview (everyBun.*property plusdefault), which is the shape the module loader itself gives the"bun"namespace.require("bun")and named imports are unaffected and still print bareglobalThis.Bun.RuntimeTranspilerCache: bump the cache version. The runtime transpiler lowering ofimport { default as x } from "bun"changed, and a release build otherwise restores the old output for files cached by a previous version (a.pilewritten by the current canary still containsvar {default: x, ...} = globalThis.Bun).Intentionally not touched: the
ImportKind::Dynamicarm next to the one changed above still prints a literalimport("bun")asPromise.resolve(globalThis.Bun), so(await import("bun")).defaultstays undefined in bundles. #37730 removes that rewrite altogether (the loader then provides the namespace), which is the right fix for it, andimport-meta.test.jscurrently asserts the old behavior.importstar/ReExportStarEntryPointAndInnerFileExternalexpected the leaked key from problem 4 ({"inner":{"b":456},"a":123,"b":456}); its non-external twin already expectedbto be absent. Updated to{"inner":{"b":456},"a":123}.Verification
New tests, all of which fail on 1.4.0 with the outputs shown in the repro and pass with this change:
test/bundler/bundler_bun.test.ts:export * from "bun"in a non-entry file for ESM and CJS output (the CJS one also checks the entry'smodule.exportsstays empty; before the fix it had 115 keys),export { default } from "bun"plusimport { default as x }for ESM output, andexport { default }/ default import /import *for CJS output.test/bundler/bundler_cjs.test.ts: non-entryexport * from "ext"with a plain external must not touch the entry'smodule.exports.test/bundler/bundler_minify.test.ts:import { default as bun, embeddedFiles } from "bun"under full minification (the default item adds a statement, so this covers semicolon insertion like the neighbouring tests).test/js/bun/resolve/import-meta.test.js: runtime transpilerimport { default as x, spawnSync } from "bun".Also run locally on this build:
bundler_bun,bundler_cjs,bundler_minify,esbuild/importstar,esbuild/default,esbuild/splitting,bundler_edgecase, thebundler_compiletests that import from"bun",resolve/builtin-esm-lazy-exports, andresolve/import-meta;--compile --bytecodein both formats of the repro above prints the right values.Fixes #20670