Conversation
|
Status Reproduced on canary (1.4.3-canary.1+09bb54630, and again on 1.4.3-canary.1+367d939d9) with the "use client";
export function Button() {
return "button";
}
The new tests cover these cases. They fail on canary and on the base branch (#42826, head 49bc7cf). They pass with the debug build of this branch: USE_SYSTEM_BUN=1 bun test test/bake/dev/bundle.test.ts -t "jsxRuntime pragma" # 1 fail
USE_SYSTEM_BUN=1 bun test test/bake/dev/production.test.ts -t "without a separate SSR graph" # 2 fail
bun bd test test/bake/dev/bundle.test.ts test/bake/dev/production.test.ts # pass |
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 that removing the jsx.development branch in wrap_value_for_server_component_reference (src/js_parser/p.rs) is only reachable through the dev server: WrapExportsForClientReference is set solely in ParseTask.rs, and the new guard keys on has_dev_server(), which is the bake.DevServer pointer — so every --app/bake production build with separateSSRGraph: false now fails at the directive before the parser runs.
Extended reasoning...
Findings were already posted inline (the --server-components-without---app unwrap path that still aborts before the new guard, and the serial test.each cost in production.test.ts). This note only records one additional check: I traced the setters of server_components (Arguments.rs, build_command.rs, bake_body.rs, bake/mod.rs) and the has_dev_server() accessor in src/bundler/options.rs, and confirmed the p.rs todo_panic! removal cannot be reached from a non-dev-server bundle except via the path the inline finding already names.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/bundler/ParseTask.rs— Users runningbun build --server-componentswithout--appstill get a process abort on any "use client" file, not the new build error. The new guard is computed at ParseTask.rs:2591 only after the.unwrap()chain ontopts.frameworkat ParseTask.rs:2570-2577 has already run, and that framework isNoneon the non-bake CLI path. Fix: evaluate the unsupported-directive check (or an explicit "requires a framework" error) before theserver_componentsmatch and replace both unwraps with a returned log error, so every "use client" abort in this mode, with or without a framework, becomes a diagnosable error.Extended reasoning...
The PR title says a "use client" module must not abort the process, and its description says the message is worded so it stays true without a framework because #38046 makes that case reachable. That case is already reachable today:
--server-componentsis a documented flag (Arguments.rs:538) and setsctx.bundler_options.server_components = true(Arguments.rs:2633), copied into the transpiler at build_command.rs:197 withoptions.frameworkleftNone. In parse_worker,use_directiveis parsed at ParseTask.rs:2388-2393 becausetopts.server_componentsis true. Target is Bun, so the match at ParseTask.rs:2567 reaches theUseDirective::Clientarm and callstopts.framework.as_ref().unwrap()at 2570-2573, panicking. Theunsupported_directivematch at 2591 is never reached. The dismissing finders confirmed the base panics identically (I verified with git show of the base file: same lines), but the PR claims "both aborts are unreachable" and the review guidance requires fixing the whole class of user-reachable panics in the same PR. Trigger: `bun build --server-components…Verification: pre-existing (the base aborts by the same route; the PR's new guard simply does not reach this path, and REVIEW.md's "fix the whole class in the same PR" rule applies because the PR's own error message is worded for exactly this no-framework case). Trigger:
bun build <entry> --target=bun --server-components(no--app, so no framework) where any JavaScript-like module in the graph starts…
There was a problem hiding this comment.
LGTM — small, self-contained conversion of two user-reachable aborts into a build error, and the serial test.each from the earlier round is now test.concurrent.each.
What was reviewed:
- The
has_dev_server()gate insrc/bundler/ParseTask.rs: tracedoptions.dev_serverto its only setters insrc/runtime/bake/DevServer.rs(server/client/ssr transpilers), so the dev server keeps bundling andbun build --appfails at the directive; it matches the siblingunique_keygate at ParseTask.rs:1364. WrapExportsForClientReferenceis set only at ParseTask.rs:2581, andwrap_value_for_server_component_referenceis reached only in that mode, so dropping thejsx.developmentbranch insrc/js_parser/p.rscannot expose a production path.- The match is exhaustive over
UseDirective, and the error path is the sameadd_range_error+ParserErrorsequence the existing "use server" arm used.
Extended reasoning...
Overview
The PR touches two Rust sites and two test files. In src/bundler/ParseTask.rs the existing "use server" early error is generalized into an unsupported_directive match that also fires for a "use client" module when the parser mode is WrapExportsForClientReference (i.e. separateSSRGraph: false) and there is no dev server. In src/js_parser/p.rs the registerClientReference wrapper now always writes self.source.path.pretty instead of branching on jsx.development and hitting todo_panic! in the other branch. Tests add a dev-server case (pragma toggling the JSX dev flag on a client module across a hot rewrite) and a production build case in two export shapes asserting the new error, its location, no dist output, and exit code 1.
Security risks
None. The change only affects diagnostics and which build-time code path is taken; no input parsing, path handling, or privilege-sensitive code is involved. The error message is a static byte string.
Level of scrutiny
Moderate. The main risk is that the new gate either fails dev-server builds that previously worked or leaves a production path that still reaches the removed branch. I traced options.dev_server: it is set on the server, client and SSR transpilers in src/runtime/bake/DevServer.rs and defaults to null in src/bundler/options.rs, so has_dev_server() is true exactly for dev-server parses, consistent with the sibling unique_key selection at ParseTask.rs:1364. WrapExportsForClientReference is assigned only at ParseTask.rs:2581 and the wrapper in p.rs is only invoked in that mode (the symbol is declared at p.rs:3480 only for it), so production can no longer reach the wrapper. The match on UseDirective is exhaustive, and the error path reuses the add_range_error + Err(ParserError) sequence from the pre-existing arm. The remaining todo_panic!("registerServerReference") is unreachable because UseDirective::Server errors first.
Other factors
The single prior comment from this system (serial test.each) was addressed in b29bec4. No CODEOWNERS entry covers the changed files. Tests live in the existing bake test files, drain stderr before asserting the exit code, use tempDir, and assert the exact message and :1:1 location. The PR is stacked on another unmerged branch, but the diff reviewed here is self-contained relative to that base. The hunt exited on dry_streak with no findings.
|
Replies to the review findings.
Serial Comments of more than one line. Done in 7eb7c36: each new comment in |
There was a problem hiding this comment.
LGTM — small, self-contained change that turns two process aborts into a reported build error, and the concurrency nit from the previous round is addressed by b29bec4.
What was reviewed:
- The
unsupported_directivematch insrc/bundler/ParseTask.rsis exhaustive and sits at the only site that assignsfeatures.server_components, so everyWrapExportsForClientReferenceparse in a non-dev-server build is rejected beforeget_astruns; the"use server"branch keeps its prior message and range. wrap_value_for_server_component_referenceinsrc/js_parser/p.rsis only reachable inwraps_exports()modes, and the server-reference mode still panics before the changed line, so dropping thejsx.developmentbranch only affects the dev-server client-reference path now covered by the new devTest.- The new production test asserts the exact message, the
:1:1location, nodistoutput, and exit code 1 for both the exported-function and export-clause fixtures;framework.serverComponents.separateSSRGraphin the message matches the real option name insrc/runtime/bake/bake.d.ts.
Extended reasoning...
Overview
The PR touches two Rust files and two test files. In src/bundler/ParseTask.rs the existing "use server" range-error branch becomes an Option<&'static [u8]> computed by a match on use_directive, adding a UseDirective::Client arm that fires only when the computed mode is WrapExportsForClientReference (i.e. separateSSRGraph: false) and topts.has_dev_server() is false. In src/js_parser/p.rs, wrap_value_for_server_component_reference no longer branches on jsx.development and always emits self.source.path.pretty as the module id. Tests add a dev-server case (pragma flips jsx.development off for one file) and a test.concurrent.each production matrix.
Security risks
None identified. The change only affects the bake bundler pipeline for "use client" modules under a non-default custom-framework mode; it converts an abort into a logged build error and otherwise emits the same dev-server output. No user-controlled data reaches a new sink; the error message is a static byte string with a range pointing at the directive.
Level of scrutiny
Moderate. I traced that features.server_components is assigned only in ParseTask.rs (line ~2563), so the new guard covers every producer of the mode the parser change relies on. The parser function has six callers in visit_stmt.rs, all gated by wraps_exports(); the WrapExportsForServerReference case still hits its todo_panic! before the changed line, and "use server" is rejected earlier in ParseTask anyway, so the removed branch is unreachable outside the dev server. The match is exhaustive (Client | None => None), and has_dev_server() is the same predicate used elsewhere in the file for dev-vs-production decisions. No debug build was present in this checkout, so I did not execute the tests; the assertions were checked by reading them against the source.
Other factors
The one inline concern from the prior round (serial test.each) was addressed in b29bec4 with test.concurrent.each; the subsequent commit only trims comments. No CODEOWNERS entry covers the changed files. The PR is stacked on an unmerged base (the "use server" error and UseDirective::range helper), but this diff only contains the commits on top of that base and does not depend on any decision beyond it landing first. The remaining todo_panic!("separate_ssr_graph=false") in bundle_v2.rs becomes unreachable with this guard, which is consistent with the PR's stated scope of not implementing production support for this mode.
With `separateSSRGraph: false` the parser wraps the exported values of a
"use client" module in `registerClientReference(value, path, name)`. It
chose `path` by `jsx.development`. A `@jsxRuntime react-jsx` pragma
turns that flag off for one file, so the dev server reached
`todo_panic!("unique_key here")`, the branch for production builds.
The parser now always writes the source path. Only the dev server
bundles in this mode. A production build reports the directive as a
build error, next to the "use server" error, because no later step
implements this mode there (#14763).
7eb7c36 to
acd83e1
Compare
|
Updated 1:57 AM PT - Sep 26th, 2026
✅ @robobun, your commit 6746fdc1ef3fbd3415ddd97de2da934d254c4cb5 passed in 🧪 To try this PR locally: bunx bun-pr 42885That installs a local version of the PR into your bun-42885 --bun |
Stacked on #42826 (three commits on top of its branch).
Problem
serverComponents.separateSSRGraph: false, a"use client"module aborts the process:panic: TODO: unique_key here (src/js_parser/p.rs:8624). Everybun build --apphits it. So does the dev server when the file has a/** @jsxRuntime react-jsx */pragma.wrap_value_for_server_component_referencepicks thepathofregisterClientReference(value, path, name)byjsx.development. The pragma turns that flag off. The other branch is thetodo_panic!.panic: index out of bounds: the len is 7 but the index is 4294967295.Fix
"use server". Both aborts are unreachable.test/bake/dev/bundle.test.ts,production.test.ts). All fail on the base branch and on canary. Self-reviewed: 7 concerns, 6 addressed (Notes).Background
"use client"marks a client component. Server code gets a reference to it. The browser loads the module.separateSSRGraph: true(built-in React) the bundler generates a proxy module for the server. Withfalsethe parser wraps the exported values inregisterClientReference.pathis what the browser imports: the source path in the dev server, a chunk URL in production (bake.d.ts:186-202). Thetruemode writes a placeholder that the linker replaces. Thefalsemode has no such code.Notes
I found this by a read of the
todo_panic!sites around #42826. There is no user report. The built-in React framework setsseparateSSRGraph: true, so only a custom framework reaches this code.The new build error:
Checked by hand on 1.4.3-canary.1+09bb54630 (release) and on a debug build of this branch. The framework is
minimalFrameworkfromtest/bake/bake-harness.ts(separateSSRGraph: false). After the rebase onto 49bc7cf (the head of #42826), I ran the tests again. The 3 new tests fail on 1.4.3-canary.1+367d939d9 and on a debug build of the base branch. They pass on a debug build of this branch.bun build --app,"use client"module withexport functionpanic: TODO: unique_key here, exit 134distbun build --app,"use client"module with onlyexport { Button }panic: index out of bounds: the len is 7 but the index is 4294967295dist"use client"module, no pragma/** @jsxRuntime react-jsx */panic: TODO: unique_key here, every route gone@jsx h,@jsxRuntime classic,@jsxRuntime automatic, or"jsx": "react-jsx"intsconfig.jsonThe second abort:
add_server_component_boundaries_as_extra_entry_pointsmakes the SSR index of each client boundary an entry point. This mode has no SSR copy, so the index isIndex::INVALID, andLinkerGraph::loadindexes the source list with it.todo_panic!("separate_ssr_graph=false")inprocess_server_component_manifest_files(bundle_v2.rs) stays on purpose, as the marker of the missing feature. The check inParseTask.rsmakes it unreachable.Why production reports an error and does not write a module id:
bake.d.ts:186-202says thatpathis the URL of the client chunk in this mode. bundler: implement server-side wrap for "use server" modules #33589 writes the source path for the dev server and production alike. A browser cannot import that path, and the build still aborts at the invalid entry point.chunk::UniqueKey, kindScb) needs the source index of the browser copy of the module. The bundler creates that copy after it parses the server copy.add_server_component_boundaries_as_extra_entry_pointspushesIndex::INVALIDas the SSR entry point.LinkerGraph::loadrewrites each server import of the module toreference_source_index, which is the browser copy in this mode.process_server_component_manifest_filesstops attodo_panic!("separate_ssr_graph=false").Self-review. The concern that I did not take: fold this commit into #42826. I stacked it. #33589 can replace the
"use server"error of #42826, and this fix does not depend on that decision. The concerns that I took:UseDirective::rangehelper. The stack uses the one from bundler: report a "use server" module as a build error instead of aborting #42826.unsupported_directiveinParseTask.rs), not two arms in the sameifchain..txtimport that starts with"use client"was reported as a client module. The JavaScript-only scan of bundler: report a "use server" module as a build error instead of aborting #42826 fixes that.todo_panic!inbundle_v2.rsnamed as left on purpose.Related:
bun build --server-componentswithout--apphas no framework. A"use client"file aborts there withpanic: called `Option::unwrap()` on a `None` value, at theunwrap()oftopts.frameworkinParseTask.rs, before the check of this PR. bun build --server-components: require the bun target and reject directives without a framework #38046 owns that abort: it removes both unwraps and reports a build error. It adds an arm to the sameifchain, so the second of the two to land needs a rebase.robobun/0eb47e53/use-server-build-errorhas the dev server half of this fix in another form (commit afab10d readsfeatures.hot_module_reloadingand keeps thetodo_panic!for production). This PR replaces that commit."use client"(no semicolon) builds underbun build --server-components, as on main. 5c5e077 in bundler: report a "use server" module as a build error instead of aborting #42826 restored that, and this branch has it since the rebase.separateSSRGraph: falsethe parser wrapsexport default,export functionandexport const. It does not wrapexport { a },export class A {}orexport { a } from, so server code gets the raw value. That is a different bug (bake: with separateSSRGraph: false, a "use client" module only gets client references for three export forms #42886). This PR does not change it.separateSSRGraph: true, the dev server can abort withpanic: Server Incremental Graph is missing component for ""when a route imports a"use client"module that imports another"use client"module. It aborted in 5 of 20 runs on 1.4.3-canary.1+367d939d9. The panic is insrc/runtime/bake/dev_server/incremental_graph.rs, which this PR does not touch. No open issue or PR names that panic.Suites run with the debug build:
test/bake/dev/bundle.test.ts,production.test.ts(with--timeout 120000, because a React production build takes about 5 s on a debug build),ecosystem.test.ts(the Svelte islands fixture uses this mode),response-to-bake-response.test.ts,test/bake/dev-and-prod.test.ts,test/bake/app-options.test.ts.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/production.test.ts