Repository navigation
Conversation
A literal `bun:X` specifier, with X not a builtin, lost its prefix while the file was transpiled. The runtime then resolved X as a package: it loaded node_modules/X, or asked the registry for X when auto-install was on. `bun build --target=bun` printed `import "X"`, and `bun add --analyze` installed X for a `#!/usr/bin/env bun` script. Three loops over the import records cut the prefix after the builtin lookup missed: Linker::link, TranspilerJob::run and BundleV2::resolve_import_records. They now cut it only when the rest is the bare name of a Node.js builtin, so `bun:fs` still loads `node:fs`. Every other `bun:` specifier stays as it is written. The runtime reports it as not found, and the bundler keeps it external under that name. The transpiler cache version goes from 34 to 35, because a cached module keeps the import path it was printed with.
Status
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughFor Bun-target imports, Changesbun: Import Resolution
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No PR-introduced issue remains identified; the known bundler limitation predates this change. The PR is mergeable after normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| 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. |
| Title check | |
The title clearly summarizes the main change: stripping the bun: prefix only when the remaining specifier is a Node.js builtin. |
| Description check | |
The description explains the problem, implementation, compatibility behavior, known limitations, cache-version change, reproduction results, and test verification. It does not use the exact template h… |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub fn is_node_builtin_after_bun_prefix(rest: &[u8]) -> bool { | ||
| // `--experimental-stream-iter` is not in the transpiler cache key, so its two names never count. | ||
| !rest.starts_with(b"node:") | ||
| && !is_stream_iter_alias(rest) | ||
| && lookup(&BUN_ALIAS_MAP, rest).is_some_and(|alias| alias.node_builtin) |
There was a problem hiding this comment.
🟡 (optional) Code importing bun:ws, bun:undici, bun:node-fetch, bun:bun or bun:node:fs now fails with Cannot find package where the base loaded Bun's built-in override. The new gate at src/resolve_builtins/HardcodedModule.rs:951 requires alias.node_builtin, but the same table also holds Bun's own overrides (ws, undici, node-fetch at HardcodedModule.rs:747-752, bun at :704) that never touch node_modules or the registry. Rejecting them serves no part of the stated goal. Fix: accept any rest that resolves in the Bun alias table (any Alias::get(rest, Target::Bun, cfg) hit, with the stream/iter exclusion), not only node_builtin entries, so every name the base routed to a builtin keeps working.
Why this was flagged
A source file containing import WebSocket from "bun:ws", require("bun:undici"), import "bun:node-fetch" or import { file } from "bun:bun" is transpiled at src/jsc/RuntimeTranspilerStore.rs:1030-1038, src/bundler/linker.rs:409-414 or src/bundler/bundle_v2.rs:6681-6685. On the base commit the bun: prefix was cut unconditionally, the bare name hit BUN_EXTRA_ALIAS_KVS (HardcodedModule.rs:747-752 ws/undici/node-fetch, :704 bun) on the next resolve, and the builtin loaded with zero filesystem or registry traffic. After the change is_node_builtin_after_bun_prefix at HardcodedModule.rs:947-952 returns false because those entries carry node_builtin: false (:708, :388), so the specifier is left as bun:ws and the resolver walks node_modules for a package literally named bun:ws, ending in Cannot find package 'bun:ws'. Under bun test the same applies to bun:vitest and bun:@ jest/globals (:805-823).
Verification: nit — acknowledged in diff: the PR description's "Downsides" says "bun:ws, bun:node:fs and bun:./file.js now fail. A code search found no public use," and test/js/node/missing-module.test.js:126-127,169-170 pins import("bun:ws") -> "Cannot find package 'bun:ws'" and import("bun:node:fs") -> "Cannot find package 'bun:node:fs'". The author's bound (no known public use) is plausible but unverifiable; the regression is real and sits beside, not inside, the stated purpose.
| // `--experimental-stream-iter` is not in the transpiler cache key, so its two names never count. | ||
| !rest.starts_with(b"node:") | ||
| && !is_stream_iter_alias(rest) |
There was a problem hiding this comment.
🟡 nit (optional): users running with --experimental-stream-iter now get Cannot find package 'bun:stream/iter' where the base branch loaded the builtin, and the comment's cache-key reason does not hold. The !is_stream_iter_alias(rest) clause at src/resolve_builtins/HardcodedModule.rs:950 guards against flag-dependent output, but lookup(&BUN_ALIAS_MAP, rest) on the next line never reads the flag, so the cut would print stream/iter in both flag states. Fix: drop the clause and the comment so bun:stream/iter and bun:zlib/iter are cut like every other bare Node builtin, leaving the flag check to the runtime resolver of the printed bare name.
Why this was flagged
A file containing import "bun:stream/iter" is transpiled by any of the three callers while bun --experimental-stream-iter is in effect. is_node_builtin_after_bun_prefix at src/resolve_builtins/HardcodedModule.rs:947-952 returns false because of !is_stream_iter_alias(rest), so the specifier stays bun:stream/iter and the runtime resolver reports Cannot find package 'bun:stream/iter'. On the base branch the prefix was cut unconditionally, the printed import was stream/iter, and Alias::get at src/resolve_builtins/HardcodedModule.rs:931 then honoured the flag and loaded the builtin. The comment at line 948 says the clause exists because --experimental-stream-iter is not in the transpiler cache key, but lookup(&BUN_ALIAS_MAP, rest) at line 951 is a plain hash lookup with no flag gating, so without the clause the cached output would be stream/iter in both flag states and no cache hazard exists; the clause only removes behaviour.
Verification: Triggering condition: a literal import/require of bun:stream/iter run under bun --experimental-stream-iter. At src/resolve_builtins/HardcodedModule.rs:947-952, lookup (line 909-914) is a plain map.get(name).copied() with no flag read. After this PR, the three callers leave the specifier as bun:stream/iter, so the runtime reports Cannot find package 'bun:stream/iter' even with the flag on.
| if let Some(rest) = import_record.path.text.strip_prefix(b"bun:") | ||
| && bun_resolve_builtins::Alias::is_node_builtin_after_bun_prefix(rest) | ||
| { | ||
| import_record.path = PFs::Path::init(rest); | ||
| import_record.path.namespace = b"bun"; | ||
| } |
There was a problem hiding this comment.
🟡 (optional) A literal import "bun:/../sql" (or bun:../sql) now loads node_modules/sql, which the base never did for a literal, defeating the PR's own goal for that spelling. The strip at src/bundler/linker.rs:409-414 (and src/jsc/RuntimeTranspilerStore.rs:1030-1038) is skipped, so bun:/../sql reaches the resolver as a bare package path; is_package_path at src/paths/lib.rs:482-496 accepts it and the node_modules join normalizes .. away. On the base the cut produced /../sql, an absolute path that never touched node_modules. Fix: before the package walk, reject any specifier that still carries a bun: or node: prefix (or whose first segment contains :), at the resolver so every entry point is covered.
Why this was flagged
Input: a string-literal import whose text is bun:/../sql or bun:../sql, arriving through any of the three rewrite loops (src/bundler/linker.rs:409, src/jsc/RuntimeTranspilerStore.rs:1030, src/bundler/bundle_v2.rs:6681). is_node_builtin_after_bun_prefix returns false for /../sql, so the text is left as written. On the base commit the loop cut the prefix, leaving /../sql, which is_package_path (src/paths/lib.rs:482-483) classifies as absolute and resolves to /sql, never node_modules. Now bun:/../sql has no leading / or ./, so src/paths/lib.rs:488-496 calls it a package path, load_node_modules joins it under each node_modules directory and the .. collapses to node_modules/sql. The package loads and runs, exactly the outcome the PR exists to stop. The dismissal said the exposure already existed on the base for a non-literal bun:X; that is a different population, and the author's own 'Still open' list confirms the literal form now reaches node_modules/sql. Remedy: have the resolver refuse bare-package lookups whose first path segment contains a colon.
Verification: Trigger: a literal import "bun:/../sql". lookup(&BUN_ALIAS_MAP, "/../sql") misses, so the specifier stays bun:/../sql; is_package_path (src/paths/lib.rs:482-496) accepts it, so load_node_modules (src/resolver/resolver.rs:2608-2611) collapses node_modules/bun:/../sql to node_modules/sql. On the base, the cut produced /../sql, never node_modules. bun:../sql is NOT affected.
| if let Some(rest) = import_record.path.text.strip_prefix(b"bun:") { | ||
| if bun_resolve_builtins::HardcodedModule::Alias::is_node_builtin_after_bun_prefix(rest) { | ||
| import_record.path = bun_paths::fs::Path::init(rest); | ||
| import_record.path.namespace = b"bun"; | ||
| } | ||
| import_record.source_index = Index::INVALID; | ||
| import_record | ||
| .flags |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Users bundling with --target=bun who write a side-effect-only import "bun:sql" get a bundle with that line silently deleted, so the runtime never reports Cannot find package 'bun:sql' as the PR promises for every other form. At src/bundler/bundle_v2.rs:6686-6689 the record is still flagged IS_EXTERNAL_WITHOUT_SIDE_EFFECTS even when the name was not stripped, and src/bundler/LinkerContext.rs:3171-3173 drops such statement imports during tree shaking. Fix: only set the flag when is_node_builtin_after_bun_prefix was true; for an unknown bun:X mark the record external but with side effects so the import survives into the output and fails at load time like the ESM/CJS named-import rows.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: bun build --target=bun (also --compile) of a file containing import "bun:sql"; with no bindings, reaching src/bundler/bundle_v2.rs:6681. The new inner if at :6682 leaves the text as bun:sql, but :6686-6689 still run and insert IS_EXTERNAL_WITHOUT_SIDE_EFFECTS. In src/bundler/LinkerContext.rs:3148-3174 a Stmt record with invalid source_index and that flag is continued, so the part is removable and the import vanishes from the output bundle. The user ships a binary that silently skips a module they asked for. The dismissal called it pre-existing because the base dropped the same import as import "sql"; but the PR changes the contract to 'the bundler keeps bun:X as written and the runtime reports Cannot find package', and the new bundler tests only cover the default-import form, so the side-effect form is the one spelling that still loses the error. The runtime loop at src/jsc/RuntimeTranspilerStore.rs:1030-1038 no longer sets the flag for non-builtins, so the two paths diverge. Remedy: move the flag insertion inside the inner if at bundle_v2.rs:6682.
Verification: Pre-existing; acknowledged in diff. Triggering condition: bun build --target=bun of a file containing a bare import "bun:sql";. src/bundler/bundle_v2.rs:6686-6689 still run unconditionally, inserting IS_EXTERNAL_WITHOUT_SIDE_EFFECTS. In src/bundler/LinkerContext.rs:3171-3173 that record hits continue, so the import part is tree-shaken out. The base branch fails the same way by the same route.
This branch has not been deployed
2 participants
Problem
bun:Ximport, with X not a builtin, runs the package X:import sql from "bun:sql"loadsnode_modules/sql. With nonode_modules, auto-install downloadssqland runs it (exit 0, no message).bun build --target=bunprintsimport sql from "sql".bun add --analyzeinstallssql.bun:off an import record on a builtin-table miss:src/bundler/linker.rs:409,src/jsc/RuntimeTranspilerStore.rs:1030,src/bundler/bundle_v2.rs:6681.Fix
bun:only before the bare name of a Node.js builtin.bun:fsstill loadsnode:fs: public code uses it (Promises returned from bun:fs/promises functions are not instances ofPromise#16883, authTagLength in createCipheriv does not work #14991).bun:specifier stays as written. A colon is not valid in an npm name, so auto-install never asks. The runtime reportsCannot find package 'bun:sql', and the bundler keeps the import external.test/js/node/missing-module.test.jsand five more files (Notes) fail on main, pass here. Self-reviewed: 5 concerns raised, 5 addressed.Background
import,require()orimport(). The loops rewrite them before printing.bun:fs. Considered a resolver reject: the name arrives there bare.Downsides
bun:ws,bun:node:fsandbun:./file.jsnow fail. A code search found no public use.node_modules/bun:Xloads forbun:X(POSIX, no registry request).Notes
Repro (loopback registry that records each request; the script runs in a directory with no
node_modules):e655c58032/madeupx,/madeupx/-/madeupx-1.0.0.tgzerror: Cannot find package 'bun:madeupx', exit 1The same holds for
await import("bun:madeupx")andrequire("bun:madeupx").Measurements (release builds of the merge base
e655c58032and of this PR, linux-x64)bun:sql(import, import(), require): 0 registry requests (was 2), exit 1 (was 0). Controls:node:sql, a non-literalbun:sqlandbun:sqliteare unchanged. With--no-installthe error now namesbun:sql, notsql.node_modules/zzzpresent: 16 rows runnode_modules/zzzon main, 0 here. The 26node:rows are identical.bun:test per non-builtin import record (objdump, path from the table miss to the next record): JS thread 14 -> 15 instructions (one register reload), worker 8 -> 8, bundler 11 -> 11. Abun:import that is not in the table pays one call toAlias::is_node_builtin_after_bun_prefix(one hash lookup).size: 89,002,375 -> 89,002,887 forbun-profile).find -newer). A cache entry that main wrote for a 7.7 KiB module withimport "bun:zzz"is rejected here. With the version header of that entry set to 35 by hand, the same binary loadsnode_modules/zzzfrom it, so the bump is needed.bun build --target=bunoutput for inputs without such an import: 0 of 622 bundles differ (311 entries fromtest/bundler/fixtures,bench/snippetsandscripts, esm and cjs, 266 output files, sha256sum).bun:zzzwithnode_modules/zzzpresent: 0 lookups ofnode_modules/zzz(was 2), 0 connects, 1openatofnode_modules/bun:zzz(ENOENT) left (ptrace syscall count).Tests (each file has a test that fails on main)
test/js/node/missing-module.test.js: a literalrequire,require.resolveandimport()get the errors that the file already pins for a non-literalbun:missing. One child process runs 15 forms withnode_modules/missingpresent (entry point, imported module, required module,.ts,.cjs, Worker) and loads nothing. An entry point with a static import exits 1. Thebun:fsforms still load.test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts: a loopback registry, by default and with--install=force. The only request is for the control package.test/bundler/bundler_bun.test.ts: esm, cjs, a#!/usr/bin/env bunentry, and--compile --bytecode.test/cli/install/bun-add.test.ts:bun add --analyzeinstallsbarand notbobaforimport "bun:boba".test/js/bun/plugin/plugins.test.tsandtest/js/bun/test/mock/mock-module.test.ts:build.module,onResolveandmock.modulesee the literal specifier.What changes, by form
import,export ... from,import(),require(),require.resolve()ofbun:X, in an entry point, an imported module, a required module, a Worker,bun -eand abun testfile. Alsobun:sql@1.0.0,bun:@scope/pkg,bun:ws,bun:bun,bun:node:fs,bun:./local.mjs, andrequire("bun:bundle").bun:plus the bare name of a Node.js builtin (bun:fs,bun:fs/promises,bun:crypto,bun:assert, ...).bun:stream/iterandbun:zlib/iterare excluded in both states of--experimental-stream-iter: the flag is not in the transpiler cache key.bun build --target=bun(esm and cjs), a#!/usr/bin/env bunentry in a default-target build,--compileand--bytecode: the output namesbun:Xas written. On main a compiled executable loadsnode_modules/Xfrom the directory it runs in.mock.module("bun:x"),build.module("bun:x")andonResolve({ namespace: "bun" })now see a literal import as it is written.Self-review. The first version deleted the cut at all three sites. A review found that
bun:<Node builtin>imports are in use:import { randomUUID } from 'bun:crypto'in TryGhost/ActivityPub,bun:fsin public test files, and the repros of #16883 (bun:fs/promises) and #14991 (require("bun:crypto")). Those names resolve to a builtin and never reachnode_modulesor the registry, so the cut stays for them. The five concerns: the allowance isbun:plus a bare Node.js builtin and nothing else. The bundler rewrites at bundle time, so a bundle withbun:fsstill runs. The allowance is one function with three callers, not new table rows. A--compile --bytecoderow exists. The still-open items are named below. The compile row is inbundler_bun.test.ts, with the other bun-target rows. The cut still writes the bare name (fs), as on main, so nothing changes for the imports it keeps.Still open, not changed here
node_modules/bun:X(ornode_modules/node:X) is loaded for that specifier. It already was for a non-literalbun:Xand fornode:X. A reject before the package lookup is a separate change with its own cost per bare-specifier resolve.import "bun:/../sql"reachesnode_modules/sql, like anyx/../sqlspecifier.import "path?v=2"asks the registry forpath. That is a different cut (the query string), reported separately.import "bun:X"orimport "node:X"of an unknown name is still dropped from a bundle (the todo testsdefault/AutoExternalBunanddefault/AutoExternalNode).onResolveplugins are still skipped for abun:import under target bun.node:arm ofLinker::linkand the differences between the two runtime loops: Report unknown node: builtins in static imports with "No such built-in module" on every transpile path #39246, Resolve static imports of unknown node: names at runtime so mock.module / build.module / plugins apply to require()d files and the entry point #39314, Keep require() of node builtins as written on both runtime transpile paths #39316, bundler: report unknown node: builtins as No such built-in module #39744.bun:dnsandbun:httpkeep loadingnode:dnsandnode:http. Say so if those names should stay free.Suites run (debug build, linux-x64): the six files above,
test/js/bun/resolve/resolve.test.ts,import-meta-resolve.test.mjs,test/regression/issue/25707.test.ts,test/regression/issue/30887.test.ts,bundler_feature_flag.test.ts,bundler_browser.test.ts,test/bundler/esbuild/default.test.ts(its twoTopLevelAwaitForbiddenRequireSourceMaptests need more than the 5 s default on a debug build and pass with a longer timeout),test/bake/dev/esm.test.ts,test/bake/dev/bundle.test.ts,test/bake/dev/production.test.ts(with a longer timeout),test/bake/dev-and-prod.test.ts. Windows and macOS were not run.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-add.test.ts