Conversation
When no target is given, `bun build` and `Bun.build()` now check every entry point (in-memory `files` first, then disk) for a `#!/usr/bin/env bun` first line, accepting LF, CRLF and EOF terminators, and default the whole build to target bun. An explicit target is left alone. This replaces the per-file override in ParseTask, which only looked at the first entry point, missed CRLF files, and retargeted that one file while its dependencies were still built for the configured target.
|
Warning Review limit reached
Next review available in: 1 minute 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 the released 1.4.0 binary and on main with |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related, but not a duplicate of either:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked whether Plugin::create(..., this.target) running before the new hashbang check in JSBundler.rs mis-tags the plugin's BunPluginTarget — it does read the pre-inference target, but the old per-file override ran even later (in ParseTask), so plugins never saw the hashbang-derived target before either; not a regression.
Extended reasoning...
The bug-hunting pass raised one candidate beyond the reported nit: that bundler plugins in Config::from_js are created with BunPluginTarget derived from this.target before the hashbang inference at line 833 runs. Verified this is not a regression — the removed target_from_hashbang override ran inside ParseTask (well after plugin setup and after BundleOptions were built), so plugin-visible target was never hashbang-aware. The actual build target (this.target) is what feeds BundleOptions, and that is now correctly set. Noting so a later pass doesn't re-explore.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after the follow-up commits and didn't find any bugs. My earlier --format=cjs concern turned out to rest on dead code in Arguments.rs and is now pinned by a test on both backends — thanks for checking. Because this changes user-facing target-inference semantics (explicit --target now fully wins over a bun hashbang) and supersedes the approach in open PRs #33859 / #33862, a maintainer should confirm the contract before merging.
What was reviewed:
has_bun_hashbangterminator handling (LF/CRLF/EOF/space/tab) and thebunx/nodenegative cases — correctly rejected.FileMap::resolve→lookupsplit preserves the Windows normalization branch and the arena-copy of the matched key.- Placement of the check in
build_command.rs(after--compile/--bytecodeforce bun, before options build) and inJSBundler.rs(afterentrypoints/filesare read, gated on!did_set_target). - Removal of the per-file override in
ParseTask.rsleaves theServerComponentsSsrbranch intact.
Extended reasoning...
Overview
Moves the #!/usr/bin/env bun → target: "bun" default from a per-file parse-time override (source index 1 only, LF/space only) to a build-wide options-layer check applied in both bun build and Bun.build(). Removes target_from_hashbang and its call site in ParseTask.rs; adds any_entry_point_has_bun_hashbang in options.rs (reads first 19 bytes via bun_sys::File, or the files map); refactors FileMap::resolve into lookup + a thin wrapper so the hashbang check reuses the bundler's own key-matching; updates docs and bun.d.ts; adds 23 test cases across CLI and API backends.
Security risks
None identified. The new file read is a bounded 19-byte read of user-specified entry-point paths that the bundler is about to open anyway; errors are silently treated as no-match. No untrusted-input parsing beyond a fixed prefix compare.
Level of scrutiny
Medium-high. The mechanical pieces are small and well-tested, but the change is a deliberate contract shift: an explicit --target=node/browser on a bun-hashbang entry now builds purely for that target (previously the entry file itself was still marked bun and got // @bun), and hashbang detection no longer sees the resolved file (so bun build ./cli without an extension won't trigger it). The PR description is transparent about both trade-offs, and the test suite pins them, but they're the kind of behavior change a maintainer should sign off on — especially given two other open PRs (#33859, #33862) fix downstream symptoms of the mechanism this PR removes.
Other factors
- My earlier inline nit (
--format=cjsCLI/API divergence) was investigated by the author, shown to be based on dead code inArguments.rs, and a regression test was added in 76bdd6a covering both backends. That thread is resolved. - The comment-cop bot flagged long doc comments; commits 6a28b96 and 4897b1d trimmed them.
- Test coverage is thorough: LF/CRLF/hashbang-only/flags/node/bunx variants, dependency target propagation, later-entry-point detection, explicit-target override with module-dedup assertion,
format: cjsinteraction, andfiles-map shadowing — all run through both the CLI andBun.build(). - The
FileMaprefactor keeps the Windows path-normalization branch and the arena-copy invariant; I checked thatresolve's only behavioral change is delegating key lookup to the newlookup/entryhelpers.
This branch has not been deployed
1 participant
Problem
#!/usr/bin/env bunshebang, the bundler defaults totarget: "bun". The implementation did something narrower, and the parts it did do were only half applied:target_from_hashbang(src/bundler/bundle_v2.rs) only accepted\nor a space after#!/usr/bin/env bun, so a CRLF file (or a file that is only the hashbang line) was built for the browser.bun build ./crlf.jsproduced no// @bunpragma while the same file with LF endings did.ParseTask.rsonly consulted it for source index 1, i.e. the first entry point.bun build plain.js cli.jsignored the hashbang incli.js.Target::Bun. Its dependencies were still resolved and printed for the configured target (browser by default), so for a script with a bun shebang and no--target:import { readFileSync } from "node:fs"in a dependency became the browser stub (typeof readFileSyncisundefinedat runtime), while the same import in the entry file worked.import "bun"in a dependency failed withBrowser build cannot import Bun builtin: "bun".// @bunpragma, which promises Bun the whole file was printed for Bun.--target=node/--target=browserfor that one file. That is the trigger behind bundler: gate @bun pragma and @bun-cjs wrapper on build target, not entry hashbang #33859 (// @bun/@bun-cjswrapper emitted into a node or browser build) and bundler: propagate known_target so a hashbang-bun entry does not split the dedup graph #33862 (the entry registers its imports in theTarget::Bundedup graph while dependencies use the configured target's graph, so a module imported by both is bundled twice andShared === ReExportedisfalse).Fix
options::any_entry_point_has_bun_hashbang(src/bundler/options.rs) reads the first 19 bytes of each entry point (theBun.buildfilesmap is consulted first, then disk) and accepts LF, CRLF, EOF, or arguments afterbun.bun build(build_command.rs) andBun.build()(JSBundler.rs) call it when the user gave no target and set the build target to bun.--compile,--bytecodeand an explicit target are untouched: an explicit setting wins over the inferred one. Nothing else disables it; in particular--format=cjs/format: "cjs"on a hashbang entry still gives a bun build (// @bun @bun-cjs) on both paths, as it did before, and a test pins that. (The documented cjs to node CLI default is not applied on main today,Arguments.rswrites it into a copy of the args thatparse()discards; that is tracked separately and, when fixed, needs to keep running after this check.)ParseTask.rsandtarget_from_hashbangare removed. With the default applied at the options layer it is redundant when no target is given (the whole build is already bun) and only harmful when one is (it was the trigger for bundler: gate @bun pragma and @bun-cjs wrapper on build target, not entry hashbang #33859 and bundler: propagate known_target so a hashbang-bun entry does not split the dedup graph #33862; both of their repros pass with this change, see the tests below). This does not changeknown_targethandling or the server-components branch.FileMap::resolveis split intolookup(returns the matched key and contents) plus a thinresolve, so the hashbang check matches entry points againstfilesexactly the way the bundler itself does.BuildConfigJSDoc, updated here) describe, it applies the target to the whole graph instead of one file, and it makes the CLI and the JS API behave the same. The cost is one small read per entry point, only when no target was given.// @bunpragma, no@bun-cjswrapper). The entry path is read as written, so an entry that only exists through resolution (bun build ./cliwithout an extension,./cli.jsnaming acli.ts, a package name) does not trigger the default; the removed override read the parsed file, but only for the first entry. Only the documented#!/usr/bin/env bunspelling is recognized, as before;env -S bunand absolute interpreter paths are not.test/bundler/bundler_bun.test.ts(bundler hashbang target default), run through bothbun buildandBun.build(): LF / CRLF / hashbang-only /bun --flagsfiles select bun and#!/usr/bin/env node/bunxdo not; a dependency'snode:fsimport survives; a hashbang on the second entry point applies to the build;--target=nodeand--target=browserare honored with the shared module bundled once and identity preserved;format: "cjs"alone keeps the bun default; afilesentry is checked instead of the file on disk. 12 of the 23 cases fail on the released binary (CRLF, hashbang-only, dependency, later entry, and both explicit-target cases, on both backends); all pass with this change.bundler_banner,bundler_files,bundler_plugin,bundler_naming,bundler_browser,bundler_edgecase,bundler_cjs,bun-build-api,cli,html-import-manifest,bundler_html_server,bake/dev/bundle,bake/dev-and-prod,integration/bun-types,cargo clippyonbun_bundlerandbun_runtime,cargo checkofbun_bundlerforx86_64-pc-windows-msvc(theFileMapchange has a Windows-only branch).Background
browser/bun/node. It selects package.json export conditions, whethernode:*andbunimports are kept or stubbed, the runtime helpers, and how files are printed. It is fixed when the options are built (BundleOptions::from_api), before any file is parsed, which is why this check has to happen in the two option-building entry points rather than in the parser.// @bunpragma: the first comment of a bun-target bundle; it tells Bun to load the file without transpiling it again. It is emitted when the entry file's per-file target is bun, so it was the visible symptom of the old per-file override.ast.target): the bundler keeps one path-to-module map per target so that, for example, a server build can also bundle browser code. A file whose target differs from its importers' therefore lives in a different map, which is how the removed override duplicated modules (bundler: propagate known_target so a hashbang-bun entry does not split the dedup graph #33862).Bun.build({ files }): in-memory files that can be used as entry points or imports and shadow files on disk with the same path; the hashbang check has to read those instead of the disk.