Repository navigation
Conversation
… of a server build An HTML entry point in a target bun/node build was only switched to the browser target when it was resolved from disk without plugins. Entry points that went through a declining onResolve plugin, an onResolve plugin returning an .html path, or the files option were enqueued under the server target, so their script chunk was compiled as server code. Move the switch into enqueue_entry_item and apply it to plugin-resolved entry points in on_resolve, so every entry point path uses the same rule.
|
Warning Review limit reached
Next review available in: 20 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 (2)
Comment |
|
Status: reproduced on bun 1.4.0 with Fix and tests are in this PR ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how enqueue_entry_item picks the target for every one of its callers (not just enqueue_entry_points_normal), a human look at the bake-production and dev-server interactions would still be worthwhile.
What was reviewed:
- Traced all six
enqueue_entry_itemcallers: dev-server callers are guarded byself.dev_server.is_none()inentry_point_target, so no behavior change there;enqueue_entry_points_bake_productionserver-side entries now get the same HTML→Browser switchprocess_resolve_queuealready applies. - Confirmed hoisting the loader lookup above the
path_to_source_index_mapdedup check is the point of the fix (dedup now happens against the browser map for HTML entries) and matches what the disk branch already did. - Checked the
on_resolveSuccess path: the switch is gated onImportKind::EntryPointBuild, so plugin-resolved non-entry imports keep their importer's target; the movedloaderbinding is used identically at both old sites. - Tests cover all four entry-resolution paths × both server targets and assert the browser export condition, not just absence of
// @bun.
Extended reasoning...
Overview
The PR fixes an inconsistency in src/bundler/bundle_v2.rs: an HTML entry point in a target: "bun"/"node" build was only switched to the browser module graph when it was resolved directly from disk. The same entry supplied via files:, or matched by an onResolve plugin (whether the plugin declined or returned a path), stayed in the server graph, so its script chunk was built with server export conditions and the // @bun pragma. The fix extracts the existing rule into entry_point_target(loader, target) and applies it inside enqueue_entry_item (covering the disk, files:, and plugin-declined fallback paths) and in the on_resolve Success path for EntryPointBuild records only. Eight new tests in test/bundler/bundler_html.test.ts cover the four resolution paths × two server targets.
Security risks
None. This is target-selection logic in the bundler; no input validation, auth, crypto, or filesystem-traversal surface is touched.
Level of scrutiny
Medium-high. The bundler's entry-point registration decides which of two module graphs a file lands in, which transpiler parses it, and which path_to_source_index_map dedups it. enqueue_entry_item has six call sites (normal, dev-server ×3, bake-production, on_resolve fallback), and moving the target rewrite inside it means every caller now sees it. The dev-server callers are protected by the self.dev_server.is_none() guard (and enqueue_entry_points_dev_server asserts dev_server.is_some()), and enqueue_entry_points_bake_production gets the same rule process_resolve_queue already applies to imported HTML — but a maintainer who knows the bake framework's expectations for server-side HTML routes should confirm that's intended.
Other factors
- The old inline block in
enqueue_entry_points_normaldid not checkdev_server.is_none(), but that function is only reached in non-dev-server builds, so adding the guard in the helper is a no-op there and correctly exempts the dev-server callers. - The loader lookup was hoisted above the dedup check; the
.unwrap_or(Loader::File)fallback is equivalent to the oldif let Some(loader)gate sinceFile != Html. - The
on_resolveSuccess change threads the recomputedtargetthroughpath_to_source_index_map,path_with_pretty_initialized,transpiler_for_target, andknown_target, and theloaderbinding moved out of the!found_existingbranch is otherwise used identically. - The PR description is unusually thorough, states the mechanism, cites the matching rule in
process_resolve_queue, and lists the neighbouring test suites that were run. No prior human reviews or outstanding comments. Given the cross-cutting nature of the change to a core bundler path, deferring rather than shadow-approving.
|
On the two callers the review flags:
The bake |
Problem
Bun.build({ entrypoints: ["./page.html"], target: "bun" })bundles the page's scripts for the browser, but the same build with any onResolve plugin whose filter matches the entry (even one that returnsundefined), with a plugin that returns the path of an.htmlfile for an entry, or with the entry supplied throughfiles:bundles them as server code: the client chunk starts with// @bun, packages resolve through the server conditions instead ofbrowser, node builtins are left as bare imports, and so on. Same withtarget: "node"minus the pragma.Target::Browserswitch lived only in the resolve-from-disk branch ofenqueue_entry_points_normal(src/bundler/bundle_v2.rs). Thefiles:branch of the same function, theEntryPointBuildfallback inon_resolve(plugin declined) and theEntryPointBuildsuccess path inon_resolve(plugin returned a path) all registered the entry under the server target, so the HTML file and everything it references were parsed with the server transpiler.Fix
entry_point_target(loader, target): an entry point with thehtmlloader in a server-side build is bundled forTarget::Browser(and the client transpiler is created); otherwise the requested target is kept. The dev server is exempt, as before.enqueue_entry_itemapplies it, which covers the disk branch (the inline block there is removed), thefiles:branch and the plugin-declined fallback.on_resolve's success path applies it toEntryPointBuildrecords only; plugin-resolved imports keep their importer's target.process_resolve_queue:loader == Html && target.is_server_side() && dev_server.is_none()-> browser graph) and to HTML entries resolved from disk. Applying it at the point where an entry point is registered makes the result independent of how the entry was resolved: in the probe below, both plugin variants now emit a chunk byte-identical to the plugin-free build (and totarget: "browser"), and thefiles:variant differs only in the path comment.import page from "./page.html"in the server entry linked to the HTML source itself and failed withNo matching export in "page.html" for import "default"instead of getting the import manifest. Listing the same HTML file twice (once on disk, once through a plugin) now dedupes to one HTML output, as it already did fortarget: "browser".test/bundler/bundler_html.test.ts("html entry point in a server-side build is bundled for the browser"): fortarget: "bun"and"node", the client chunk of an HTML entry resolved from disk, through a declining onResolve plugin, through an onResolve plugin returning an.htmlpath, and throughfiles:picks thebrowserexport condition and has no// @bunpragma. On the release build the six plugin/files:cases fail, the two disk cases pass; all pass with this change.html-import-manifest,bundler_files,bun-build-api,bundler_plugin,bundler_html_server,standaloneand the bakedev/html,dev/plugins,dev/production,dev-and-prodtests against the debug build: no failures related to this change.Background
bun/node) build that contains HTML bundles two module graphs: the server graph, and a browser graph for the HTML and its scripts/styles. Each graph has its own path -> source index map (path_to_source_index_map(target)) and its own transpiler; the browser one is created lazily byensure_client_transpilerand must exist before a browser-target parse task is scheduled, because parse workers clone it fromBundleV2::client_transpiler.ParseTask::known_targetselects which transpiler parses a file; the HTML file's target is inherited by every script and stylesheet it references, so the entry's target decides the target of the whole client chunk.// @bunis emitted for chunks whose entry was parsed for the bun target.enqueue_entry_points_normal(resolver orfiles:), and, when a plugin's onResolve filter matches the entry specifier,on_resolvewith anImportKind::EntryPointBuildrecord, which either falls back to the resolver (plugin returned nothing) or registers the path the plugin returned.Probe on bun 1.4.0 (page.html with a script tag for app.js)
With a package exporting
{ browser: "./browser.js", default: "./server.js" }, the plugin andfiles:variants resolveserver.jsunder bothtarget: "bun"andtarget: "node"; the baseline resolvesbrowser.js.Two neighbouring bugs found while probing are reported separately and are not changed here: a
files:entry is "ModuleNotFound" when a matching onResolve plugin declines it (the fallback never consults the file map), and a server file importing an.htmlfile supplied throughfiles:gets no import manifest.