Repository navigation
Conversation
… output
For target bun, cjs and iife output turn an external import statement
into a require() call. The printer dropped the statement's `type`
attribute there, so the runtime picked the loader from the file
extension. The call now carries it as `require(path, { type })`, which
the runtime already reads.
ImportRecord.loader now holds only the loader that the attribute names.
resolve_import_records no longer stores the loader it resolves for a
bundled file there, and the `module.exports = require(x)` redirect
takes the inner record's loader with its path. ESM output no longer
prints a stray `with { type: "jsx" }` on an import that was redirected
to an external module.
For text, md and sqlite the `__toESM(` call closes without the node
mode flag: require() returns an ES module namespace for these types.
A typed import is rare. With the three print calls inline, the closing parenthesis of every external require() stopped being inlined: 45 more instructions per call for an import with no attribute.
…copy
- repl: the `await import()` that replaces a typed import statement now
gets `{ with: { type } }`.
- bundler: the `module.exports = require(x)` redirect is back to main. With
the resolved-loader stores gone the shim's record has no loader to leak.
- The types whose require() result is a generated ES module are a
wildcard-free match on Loader, pinned against the runtime by a test.
- Comments under src/ are one line each.
The compile variants print the same cjs chunk as the bytecode case, and the import-attributes file is too slow for a debug build to run whole.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 seconds. View limit details
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 |
|
Updated 5:16 AM PT - Oct 21st, 2026
❌ @robobun, your commit ba4b1ec has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46294That installs a local version of the PR into your bun-46294 --bun |
|
Status: fix pushed, waiting for CI. How I reproduced it, on 1.4.3-canary.1+367d939d9 and on main: bun -e 'const {Database}=require("bun:sqlite"); const db=new Database("my.db"); db.run("create table t(x)"); db.run("insert into t values (1)"); db.close()'
printf 'import db from "./my.db" with { type: "sqlite" };\nconsole.log(db.query("select x from t").get());\n' > index.ts
bun build --target=bun --format=cjs ./index.ts --outfile out.cjs && bun out.cjsThe output has With this branch every one of those prints |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the two deleted import_record.loader = Some(...) stores in src/bundler/bundle_v2.rs: the remaining readers of record.loader (the plugin onResolve result paths around lines 2723 and 2979, and the sqlite external check at 6748) all run before the deleted store sites or fall back to the extension-derived loader, so nothing downstream loses the resolved loader. The with { type } and FetchParameters rewrites in the printer are behavior-preserving for every loader variant per the round-trip unit test, and non-bun targets stay unchanged via the IS_BUN_PLATFORM gate.
Extended reasoning...
The change touches the JS printer's require() lowering, the bundler's import-record resolution, the REPL import transform, and the Loader enum, with new itBundled and REPL tests; it touches no security-sensitive surface. Three inline findings were posted (missing --compile coverage, duplicated object-building blocks in the REPL helper, and a pre-existing gap in the , 1 exemption), so a human look is already signalled; this note only records that the bundle_v2 store deletions and the printer match-table consolidation were checked and found behavior-preserving.
| for (const bytecode of [false, true]) { | ||
| itBundled(`bun/sqlite-type-attribute-cjs${bytecode ? "-bytecode" : ""}`, { |
There was a problem hiding this comment.
🟡 (optional) Maintainers get no automated coverage of the --compile path this PR names as the user-visible trigger, so a standalone-executable regression would pass CI. The PR text says the defect shows when the docs sqlite example is built with --compile --bytecode and that this variant was only run by hand, while test/bundler/bundler_bun.test.ts:94 covers only format: "cjs" and bytecode without compile. Fix: add the compile variants alongside the existing cases (itBundled already accepts compile: true with bytecode), so cjs, iife, --bytecode and --compile --bytecode are all exercised against a runtime .db file.
Why this was flagged
The new cases at test/bundler/bundler_bun.test.ts:94-108 iterate bytecode in [false, true] with format: "cjs" and outdir: "/out", and the iife case at test/bundler/bundler_bun.test.ts:109 uses format: "iife"; none sets compile. Standalone executables load modules from the embedded /$bunfs graph through src/standalone_graph/StandaloneModuleGraph.rs, a different module-loading path from a cjs file on disk, and require("./my.db", { type: "sqlite" }) is resolved relative to that virtual root there. The PR description states the --compile --format=cjs and --compile --bytecode builds of the docs example were run by hand only. On the base branch this path also has no test; after the merge a future change that breaks the second require() argument inside a compiled executable would still pass the suite. itBundled supports compile: true (test/bundler/expectBundled.ts:193) together with bytecode, so the variant can be added without new harness code.
Verification: The PR description names --compile --bytecode as where the defect shows; the manual-run claim is not something the test suite enforces. The new cases in test/bundler/bundler_bun.test.ts lines 94-116 set format: "cjs", bytecode in [false,true] and format: "iife", but none sets compile. Any regression confined to the standalone-executable path would not be caught.
| let mut attributes = BumpVec::<G::Property>::with_capacity_in(1, bump); | ||
| attributes.push(G::Property { | ||
| key: Some(self.new_expr( | ||
| E::String { | ||
| data: b"type".into(), | ||
| ..Default::default() | ||
| }, | ||
| loc, | ||
| )), | ||
| value: Some(self.new_expr( | ||
| E::String { | ||
| data: loader.import_attribute_type().into(), | ||
| ..Default::default() | ||
| }, | ||
| loc, | ||
| )), | ||
| ..Default::default() | ||
| }); | ||
| let with = self.new_expr( | ||
| E::Object { | ||
| properties: G::PropertyList::from_bump_vec(attributes), | ||
| is_single_line: true, | ||
| ..Default::default() | ||
| }, | ||
| loc, | ||
| ); | ||
| let mut options = BumpVec::<G::Property>::with_capacity_in(1, bump); | ||
| options.push(G::Property { | ||
| key: Some(self.new_expr( | ||
| E::String { | ||
| data: b"with".into(), | ||
| ..Default::default() | ||
| }, | ||
| loc, | ||
| )), | ||
| value: Some(with), | ||
| ..Default::default() | ||
| }); | ||
| self.new_expr( | ||
| E::Object { | ||
| properties: G::PropertyList::from_bump_vec(options), | ||
| is_single_line: true, | ||
| ..Default::default() | ||
| }, | ||
| loc, |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get two near-identical 20-line "build a one-property object" blocks in the new helper where one shared helper would do. In src/js_parser/repl_transforms.rs:729-773 the attributes block and the options block differ only in key string and value expr. Fix: extract a helper that takes (key, value, bump, loc) and returns the single-property E::Object, and call it twice (type → string, with → inner object), which also matches the existing repl_wrap_expr_in_value_object style in the same file.
Why this was flagged
Triggered whenever a maintainer reads or changes repl_import_type_options in src/js_parser/repl_transforms.rs:722-773; no runtime caller or input is involved. The function builds { type: "<name>" } at lines 729-753 and { with: ... } at lines 755-773 with the same BumpVec::with_capacity_in(1, bump) + G::Property push + E::Object construction, differing only in the key literal and the value expr. The base branch has no such helper, so there is no behavioral difference for users; this is a code-shape duplication within the diff only. No safeguard applies; it is a style nit under the review rule that a repeated multi-line block in one diff should become a named helper.
Verification: nit. Triggered only when a maintainer reads or edits the new helper; no runtime behavior is affected. repl_import_type_options at /home/claude/bun/src/js_parser/repl_transforms.rs:723-775 builds { type: "<name>" } at lines 729-754 and then repeats the identical shape at lines 755-774 for { with: <inner> }, differing only in the key literal and the value expr.
| if wrap_with_to_esm { | ||
| self.print_to_esm_suffix(); | ||
| // `isNodeMode` would make a generated module's whole namespace the default import. | ||
| if type_attribute | ||
| .is_some_and(|loader| loader.require_returns_generated_es_module()) | ||
| { | ||
| self.print(b")"); | ||
| } else { | ||
| self.print_to_esm_suffix(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: users bundling for target bun in cjs/iife from a "type": "module" project still get a namespace object as the default import of an external .txt or .md file. The , 1 exemption at src/js_printer/lib.rs:2999 keys on the type attribute only, so import text from "./a.txt" with --external "*.txt" still prints __toESM(require("./a.txt"), 1) and text.trim() throws. Fix: decide isNodeMode from the loader the runtime will actually pick for the external path (the attribute, else the extension via the loader table), or drop , 1 for every external require() on target bun, covering typed and untyped text, md and sqlite alike. [also at: src/js_printer/lib.rs:3005 - pre-existing: users bundling for target bun with cjs/iife output and "type": "module" still get a namespace, not the string, as the default import of an external untyped .txt/.md file.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Input: a project with package.json "type": "module", built with target bun and format cjs or iife, containing import text from "./a.txt" (no attribute) where *.txt is external, run through bun build / Bun.build. The record has loader None, so at src/js_printer/lib.rs:2999 type_attribute.is_some_and(...) is false and src/js_printer/lib.rs:3004 prints , 1) because input_module_type is Esm (src/js_printer/lib.rs:3074). At run time Bun's require() of a .txt file returns a generated ES module namespace, and __toESM with isNodeMode=1 sets default to that whole namespace, so text is an object and text.trim() throws TypeError. The base branch fails the same way; the PR fixes only the sibling where a with { type } attribute is present, so the extension-selected loaders (.txt, .md, and any --loader .ext:sqlite) stay broken. A fix needs the loader the runtime will choose, which the linker can derive from the record path's extension for external records.
Verification: pre-existing; acknowledged in diff: the PR description says "Still wrong in cjs output under "type": "module": an untyped .txt, .md or .sqlite import ... This PR leaves that choice open". At src/js_printer/lib.rs:2999-3000 type_attribute is None for an untyped import, so print_to_esm_suffix() prints , 1); __toESM(mod, 1) in src/runtime.js:67-68 makes the namespace the default, so text.trim() throws TypeError.
Problem
import db from "./my.db" with { type: "sqlite" }intorequire("./my.db"). The runtime then picks the loader by extension:error: Expected ";" but found "format".bun replfails the same way.require()branch ofprint_require_or_import_expr(src/js_printer/lib.rs) and the REPL lowering (src/js_parser/repl_transforms.rs) never readrecord.loader.Fix
require(path, { type }), which the runtime already reads. The REPL printsimport(path, { with: { type } }).resolve_import_recordsno longer stores each bundled file's loader inrecord.loader. Fortext,mdandsqlite,__toESM(closes without, 1.test/bundler/bundler_bun.test.ts,bundler_cjs2esm.test.ts,test/js/bun/repl/repl.test.ts(11 cases fail on 1.4.3). Also splitting, loader, html, bake.Background
loaderas thetypeattribute.__toESM(value, 1)makesvaluethe default import. For these typesrequire()returns an ES module namespace, which has its own default.Downsides
with { type: "jsx" }after amodule.exports = require("ext")redirect., 1list is a stopgap: with"type": "module", an external ES module still gets the namespace as default.type: +4 instructions per printedrequire()(615 to 619), 415 of 415 outputs identical,.text+512 bytes.Notes
Reach
docs/bundler/executables.mdxwith the--compile --bytecodecommand of the same page.--format=cjs,--format=iife,--bytecode, each also with--compile). The import stays external, and itstypeis not the loader that the file extension selects.embed: "true"example fails at a different site (bundler: use require() for embedded sqlite modules in cjs output #38173).--format=esm,new Database("./my.db"), or a hand-writtenrequire(path, { type }).Changes by file
src/js_printer/lib.rs: the externalrequire()arm prints, { type: "<name>" }throughprint_type_attribute. The helper is out of line on purpose. With the three prints inline, the closing)of every externalrequire()was no longer inlined (615 to 660 instructions per call).Loader::import_attribute_type.src/ast/loader.rs:import_attribute_typehas a round-trip unit test for every variant (it runs under Miri).require_returns_generated_es_moduleis a match with no wildcard, so a new loader must be classified.src/bundler/bundle_v2.rs: the twoimport_record.loader = Some(import_record_loader)stores are gone. The local still feedsresolve_task.loaderandis_html_entrypoint. Only the printer read the stored value.src/js_parser/repl_transforms.rs: theimport()gets{ with: { type } }when the record has a loader. An import with no attribute prints as before.The
, 1rule needs a maintainer decision, 1(node mode) to__toESM(require(...))for importers that are ES modules by type.text,mdandsqlite,require()returns the namespace of a module that bun generates. Node mode makes that namespace the default import:TypeError: import_my.default.query is not a function.typeattribute. A test pins the runtime table behind the list.bun:specifiers in the sameif. The PR that merges second needs a rebase."type": "module": an untyped.txt,.mdor.sqliteimport, and any external ES module. The default import is the namespace there., 1on an externalrequire()) covers those cases. It also changes the default import of a CommonJS file with__esModulein such a project. This PR leaves that choice open.Not in this PR
export ... from "x" with { type }: the parser does not record the loader for re-exports (src/js_parser/parse/parse_stmt.rs), in every output format. Open Carry import attributes through the parser, printer, bundler and runtime; add the bytes loader #40836 has that change. No tracking issue exists.Try require("<path>") insteaddoes not mention the type. No tracking issue exists.import deferof an external import is eager in cjs output (Preserve import defer semantics when bundling #31374).import()of one file are two module instances.require()andimport()key the module differently at run time. No tracking issue exists.Related open PRs
with { type: ... }on external hoisted frommodule.exports = require(...)#35692: the straywith { type: "jsx" }it fixes is gone here, because the stored loader is gone. Two of its tests are in this PR. The redirect itself is unchanged.import_record.loaderafter the store that this PR removes. After a rebase it must read the localimport_record_loader.Measurements (linux-x64, release builds with LTO of main
ea6a2c7b7cand of this branch, built at the same path)type: 0 of 415 files differ (sha256sum).--no-bundleoutput and a generated 400-module app, with and without--splitting.module.exports = require("the-external")file, ESM output: 3 of 23 lines change. Each loseswith { type: "jsx" }.__require("the-external")does not change.require()with notype(gdbstepi): 615 to 619 instructions in the bundler. With__toESM: 1070 to 1074. In the runtime transpiler: 720 to 724.type: 3217 to 3224 instructions.resolve_import_records: loader stores per bundled record 1 to 0 (objdump, 2 to 0 store sites)..text+512 bytes,.rodataunchanged.js_printersymbols -668 bytes,js_parsersymbols +3,148 bytes.ImportRecordstays 96 bytes.EXPECTED_VERSIONof the transpiler cache is unchanged, and--no-bundleoutput is identical.catch syscall).Tests
require()of an in-memory file prints one argumenttypegets no options, 1listbundler_bun,bundler_cjs2esm,bundler_files,bundler_splitting,bundler_loader,bundler_cjs,bundler_plugin,bundler_edgecase,bundler_html,bundler_html_server,html-import-manifest.loader,default,metafile,importstarandsplittingsuites,import-attributes,transpiler,repl,repl-transform, and bake devbundle,html,esmandcss.edgecase/DeepImportDiamondDAGhits its 60 s limit on this debug build (95 s for 20,000 modules). Release builds of main and of this branch take the same time and give identical output.--compile --format=cjsand--compile --bytecodeof the docs example: run by hand, both print the row.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/repl/repl.test.ts