Repository navigation
Conversation
…in CommonJS output
An ESM-by-type importer (.mjs, .mts, "type": "module") printed
__toESM(require("bun:sqlite"), 1) under --format=cjs and --format=iife.
isNodeMode ignores __esModule, so the default import was the whole
require("bun:sqlite") object and new Database() threw. Bun's loader
makes an own default property of a builtin's exports the ES default
export for every importer, so a bun: specifier never takes isNodeMode.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Bun built-in import wrapping src/js_printer/lib.rs, test/bundler/bundler_cjs.test.ts |
The printer omits the Node-mode argument when a wrapped external import path starts with bun:. Tests check bun:sqlite default and namespace behavior across importer types, targets, and output formats, and check Node-mode handling for node:fs. |
Possibly related PRs
- oven-sh/bun#46736: Changes
__toESMNode-mode selection for CommonJS data-file imports, which is also the behavior affected by this PR. - oven-sh/bun#40940: Changes module-type detection that informs the
isNodeModedecision covered by this PR.
Suggested reviewers: jarred-sumner
Priority:
Merge Risk: 
3452f
-
The change corrects Bun built-in default imports. No merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: preserving the default export for external bun: modules in CommonJS output. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description is complete and relevant. It explains the problem, cause, fix, scope, known limitations, performance impact, and verification results. It does not use the template headings exactly, bu… Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, cause, fix, scope, known limitations, performance impact, and verification results. It does not use the template headings exactly, but it provides the required information, including how the code was verified.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Status: ready for review. The head is 3452faa, with main 8a0e49e merged. Reproduced on bun 1.4.3-canary.1 and on builds of main 3396aea and 8a0e49e. Put The three new tests in Scope: this PR restores the 1.4.0 result for |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow, well-reasoned regression fix restoring 1.4.0 behavior for bun: default imports.
What was reviewed:
- The
bun:prefix check usesstrings::has_prefix_comptimeper src/CLAUDE.md and only touches the external-require()emission arm; the otherprint_to_esm_suffix()sites (lines 2794, 2887) are guarded byrecord.source_index.is_valid()so they never see externalbun:records, and the dynamic-import()arm keeps a realimport(...)so Bun's loader already returns the correct namespace there. - Tests cover the variant matrix (
.mjs/.mts/type: module×cjs/iife×target: bun/node), assert both bundle text and runtime stdout, and include anode:fscontrol showing non-bun:externals still get, 1. - No CODEOWNERS cover the changed paths.
Extended reasoning...
Overview
This PR fixes a 1.4.1 regression where bun build --format=cjs (or iife) of an ES-module-typed importer wrapped require("bun:sqlite") in __toESM(..., 1), making the default import resolve to the whole exports object instead of the Database class. The fix is a 7-line branch in src/js_printer/lib.rs (external-require() arm of print_require_or_import_expr) that emits a bare ) instead of calling print_to_esm_suffix() when the record's specifier starts with bun:. Three itBundled cases are added to the existing test/bundler/bundler_cjs.test.ts.
Security risks
None. This changes only whether a , 1 argument is emitted to the __toESM runtime helper for bun:-prefixed specifiers in bundled output. No parsing of untrusted input, no auth/crypto/permissions surface, no memory-safety implications.
Level of scrutiny
Moderate — the printer is hot bundler code, but the change is a single narrowly-gated branch. I checked the other three wrap_with_to_esm emission sites: two (lines 2794 and 2887) are inside the record.source_index.is_valid() internal-module path, which bun: builtins never enter (they are always external), and the third (line 3040) is the external dynamic-import() path, which keeps a real import("bun:...") at runtime where Bun's ESM loader already produces the correct default. So the single site changed is the whole class for this bug. The prefix check uses the mandated strings::has_prefix_comptime helper, and the comment is a load-bearing why-comment per REVIEW.md.
Other factors
Tests follow the harness conventions: added to the existing bundler_cjs.test.ts, use itBundled, share a fixture across the three importer-type × format × target combinations, assert exact bundle text (__toESM(require("bun:sqlite")) with no , 1) plus runtime stdout, and the target: node case includes a node:fs control asserting , 1 is still emitted for non-bun: externals — so the change is proven not to over-apply. The PR description documents that the author diffed all builtins and only bun:sqlite diverges between the two __toESM modes. Exit reason was dry_streak and no CODEOWNERS cover these paths.
|
Updated 9:09 PM PT - Oct 26th, 2026
✅ @robobun, your commit 3452faac8e8f02854cb5b83c88a63d47c11a72d7 passed in 🧪 To try this PR locally: bunx bun-pr 41953That installs a local version of the PR into your bun-41953 --bun |
…builtin-default-import-cjs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Only Bun loads a `bun:` module, and its loader makes an own | ||
| // `default` of the exports the ES default for every importer. | ||
| // `isNodeMode` would make it the whole exports object. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Agreed. The test is a special case for one specifier, and the cause is wider than bun: modules.
The two comments of 2026-10-27 on this PR list three rules for this arm. Under the first and the third rule this test and its comment are deleted. The third rule is built on robobun/811841e7/external-import-no-mode-target-bun. Under the second rule the linker makes the decision and the printer reads a flag.
The arm stays as it is in this PR until a maintainer picks the rule. I leave this thread open until then.
|
A linker-level version of this fix is ready on a branch. It is not a second PR. It is here for the shape that the decision below selects. Branch What the branch does
The decision that selects the shapeMust an import that is not bundled ever get Default import from an
Notes
|
|
The third option in the table above ( What the branch does
Numbers for the decisionRelease builds of main c643ea8 and of the branch, linux-x64.
I also took the default import of each of the 1192 package versions in a clean test store, from a
One project outside this repository reports the What gets worse or stays broken on the branch
RecommendationTake the third option now. It changes no output outside target bun, and it is the only one of the three that fixes external ES module and dual packages there. The first option is the same change without the target test: it also fixes the node target and flips Tests and relation to the other open changes
|
Problem
bun build --target=bun --format=cjsofimport Database from "bun:sqlite"in an ES module by type (.mjs,.mts,"type": "module") prints__toESM(require("bun:sqlite"), 1).new Database()throwsTypeError: Object is not a constructor (evaluating 'new import_bun_sqlite.default(":memory:")'). The build exits 0. Also with--format=iife,--bytecode,--target=node.__toESM(require(..))with, 1(isNodeMode) for such an importer (src/js_printer/lib.rs:2994).isNodeModeignores__esModule, whichrequire("bun:sqlite")sets (src/js/bun/sqlite.ts:742).Fix
require()path prints no, 1when the specifier starts withbun:. The default import isexports.default, as in 1.4.0.bun:module, and Bun's loader makes an owndefaultof a built-in's exports the ES default export for every importer (generateInternalModuleSourceCode,src/jsc/bindings/ModuleLoader.cpp:72).test/bundler/bundler_cjs.test.ts(3 new tests, all fail on main). Other suites: Notes.Background
__toESM(mod, isNodeMode)builds the ES module view of arequire()result. WithoutisNodeMode,defaultismod.defaultwhenmod.__esModuleis set. With it,defaultismod, as in Node.isNodeModeper import record. It waits on a decision about externals (see the comments). This check is the smallest change that restores 1.4.0.Downsides
type: "sqlite"file import and the default import of an external ES module package still get, 1and give the module object.bun:imports lose, 1too: 3 bytes each, same value (12 of 258 corpus outputs)..text+512 bytes. One printed__toESM(require())runs 876 instructions, 996 before.Notes
Scope and the open decision
This PR restores the 1.4.0 result for
bun:modules only. The same line of #41150 also changed the default import of atype: "sqlite"file and of an external ES module package, on Bun and on Node. Whether an externalrequire()must ever get, 1from an importer that is an ES module by type needs a maintainer decision. The table for that decision, and a linker-level version of the fix on a branch, are in #41953 (comment).Measurements (linux-x64, release builds of main 8a0e49e and of this branch at 3452faa, same machine and configuration)
.js,.mjs,.mts,"type": "module"and"type": "commonjs"importers, 20,000 user-land externals, React 18.3.1 server rendering, every entry ofbuiltinModulesas a default and a star import, and a sqlite file import. Targets bun, node and browser. Formats esm, cjs and iife. Plain and--minify. 238 files are byte-identical. 8 change forbun:sqlite. 12 change only becausebun:ffi,bun:jsc,bun:test, an unknownbun:specifier, andbun:fsunder--target=nodelose, 1.bun:sqliteis the only built-in where__toESM(value, 1).defaultand__toESM(value).defaultdiffer, so the 12 other files keep their values..text82,158,473 to 82,158,985 bytes (+512)..rodatadoes not change.print_require_or_import_expr(5 instantiations) 39,870 to 40,068 bytes.stepi): one call ofprint_require_or_import_exprthat prints__toESM(require("pkg-0"), 1)runs 996 instructions on main and 876 on this branch. The output is byte-identical. The compiler lays the function out differently.perf_event_opencounter, 20 interleaved runs: 355,885,852 to 354,263,988 (-0.456 %).perfandvalgrindare not installed on the machine that ran this.Verification on the merged head
test/bundler/bundler_cjs.test.tspasses 65 of 65.bundler_bun,bundler_edgecase,regression/issue/03844.functionfor the repro below. Main 8a0e49e printsobject.The account from the first version of this PR
Repro (1.4.1, 1.4.2, 1.4.3, main):
The same file named
a.tswithout"type": "module"was fine: it printed__toESM(require("bun:sqlite")).--format=esmkeeps theimportstatement and was fine too.Why the specifier prefix and not
ImportRecordTag::Builtin: the tag is only set under--target=bun, where it also coversnode:*and the packages Bun overrides (ws,node-fetch,undici, ...). Under--target=nodeabun:sqliteimport stays external with tagNoneand had the same bug, and the output can only run itsrequire("bun:sqlite")on Bun. The prefix covers both targets and leavesnode:*output byte-identical to esbuild's.Matrix used to find affected modules, run on bun 1.4.3: for each builtin
m, compare(await import(m)).defaultwith__toESM(require(m), 1).defaultand__toESM(require(m), 0).default. Mismatches:bun:sqlite(node mode only) andbun:wrap(no default export at all, soimport d from "bun:wrap"is a SyntaxError unbundled). The overridden packages (ws,node-fetch,undici,isomorphic-fetch,@vercel/fetch,abort-controller) agree in both modes.Not changed here, for a maintainer to judge: for a user-land CommonJS file,
bun runhonors__esModulefor every importer and keys only on the CommonJS file's own package.json"type"(populateESMExports,src/jsc/bindings/JSCommonJSModule.cpp:1012). The bundler keys on the importer, as esbuild and Node do. Soentry.mjsimporting a Babel-styledep.cjsprintsfunctionunderbun entry.mjsandobjectafterbun build --target=bun. #41150 chose the esbuild rule on purpose. This PR only takesbun:modules out of it.import Bun from "bun"under--format=cjsisglobalThis.Bun.default, which isundefined. That is older than 1.4.1 and a different code path (ImportRecordTag::Bun), so it is not touched here.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file