Repository navigation
Conversation
|
Updated 9:20 AM PT - Oct 24th, 2026
✅ @robobun, your commit 79a6f4d4cbda4a68667be75e31dceb2b086925f4 passed in 🧪 To try this PR locally: bunx bun-pr 46702That installs a local version of the PR into your bun-46702 --bun |
|
Status Reproduced on the base branch of this PR (#46652), with release and debug builds. A bake server route that imports a package with #46652 now corrects the main fields of these graphs, so the string CI at 79a6f4d (build 132017): every lane that ran passes. That is every Linux lane, Windows 2019 x64, Windows 11 aarch64 and macOS aarch64. The two macOS x64 shards did not get an agent: the first pair expired after 88 minutes in the queue, and the retried pair is in the queue. This PR merges after #46652. PR: #46702 |
There was a problem hiding this comment.
Beyond the inline findings, I also checked two things that turned out not to be issues: the directory-change retry busts the dir cache only through the server resolver, but Resolver::dir_cache is a process-global static (src/resolver/resolver.rs:804), so the new client-graph retry sees the same busted entries; and the other options Transpiler::init now derives from the bun target (inlining, import_path_format) are read only by the legacy non-bundle_v2 transpile path, so they do not change bake output.
Extended reasoning...
The change moves the bake server/SSR transpiler target into Transpiler::init, resolves the server entrypoint with the server resolver, and threads the importer's graph into the directory-watch retry; the three inline findings cover the resulting resolution regressions for "use client" modules and the new .node loader in the server graph. It touches no security-sensitive surface. The cache-bust and derived-option checks above were examined from the resolver and options source and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (3):
- 🔴
src/runtime/bake/config/transpiler.rs:28—Bake apps with a "use client" component that imports a browser-only package now fail to build with Could not resolve, w… - 🔴
src/runtime/bake/config/transpiler.rs:29—Bake users whose route imports a package with a "use client" entry and a separate browser build now ship the node build… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
|
Review follow-up, at d501d86. Changed:
Not changed, with the measurements in each thread and in the PR body:
|
facba87 to
0c2ef95
Compare
d501d86 to
5b5d0c5
Compare
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 nit, I audited the other state BundleOptions::from_api derives from the target now that the server/SSR transpilers are built for bun: inlining and import_path_format are only read on the non-bundle_v2 transpile path, out_extensions, tree_shaking and minify_syntax are overwritten later in init_transpiler, and polyfill_node_globals now correctly becomes false for server graphs. Also checked the new assume_init_mut on client_transpiler in hot_reload_event.rs: both slots are written in DevServer::init before the box is assumed initialized, and the Ssr arm mapping to server_transpiler avoids the aliased ssr_transpiler when separateSSRGraph is off.
Extended reasoning...
The change moves the bake target into Transpiler::init (src/runtime/bake/config/transpiler.rs), resolves serverEntryPoint with the server resolver, threads the importer graph through Dep so the directory-watch retry uses the matching transpiler, narrows BundleOptions::main_fields to pub(crate), and adds dev-server and bun build --app tests. No security-sensitive surface is touched. One nit finding is posted inline; the earlier rounds' findings about client-component resolution were self-resolved by the author, so a human look at that design choice remains worthwhile.
|
Review follow-up, at 2116402.
|
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the test-only commit addresses the separateSSRGraph: false gap raised on test/bake/dev/esm.test.ts. Because this changes how every bake server/SSR graph resolves packages, with build failures the description itself lists as downsides, a maintainer should still weigh the trade-off before merging.
What was reviewed:
- Every side effect
BundleOptions::from_apiapplies fortarget: bunbeyond main fields: conditions are fully rebuilt attranspiler.rs:63,env.behavioris reset todisablebeforeconfigure_defines,tree_shaking/minify_syntaxare overwritten later,import_path_formatandinliningare not read on the bundle_v2 path, andpolyfill_node_globalsnow correctly turns off for the server resolver. - The dev-server retry now uses the client transpiler for client-graph deps; the resolver's
dir_cacheis a process-wide singleton, so busting it through the server resolver still covers the client retry. Dep.graphis set at both constructor sites; theDefaultvalue is only used to clear freed slots.
Extended reasoning...
The change switches bake's server and SSR transpilers to the bun target inside Transpiler::init (transpiler.rs), resolves the framework server entry with the server resolver (framework.rs), records the importer graph on directory-watch deps so failed-import retries use the matching transpiler (directory_watch_store.rs, hot_reload_event.rs), narrows main_fields to pub(crate), and adds dev and production tests plus harness fixtures. It touches no auth, crypto, or injection surface. Deferring rather than approving because the PR is stacked on another unmerged branch, intentionally changes module-resolution behavior for every bake server graph with acknowledged regressions (browser-only deps of "use client" components now fail the build, .node loader in server graphs), and the earlier inline concerns on those points were resolved by the author rather than an independent reviewer.
0c2ef95 to
0f1a0e9
Compare
`Framework::init_transpiler` built every transpiler for the default target, the browser, and then assigned the target and the main fields of the graph. The rest of what `Transpiler::init` derives from the target stayed the browser's: the extension order, the loaders, and the resolver's switch for package.json "browser" maps. A server route resolved a package through its "browser" map. - `init_transpiler` passes the target of the graph to `Transpiler::init` and no longer assigns the target and the main fields after it. `init` turns environment inlining on for the bun target, so the function sets it off when the config has no `env` option. - `BundleOptions::main_fields` is `pub(crate)` again. No other crate assigns it now. - `Framework::resolve` resolves `serverEntryPoint` with the server resolver. It used the client resolver. - The dev server tries a failed import again with the resolver of the graph that has the importer. It used the server resolver for every graph, which no longer reads "browser" maps.
…R graph With `separateSSRGraph: false` the route now returns the value inside the client reference, and the test compares it with the files that a resolver for bun takes.
2116402 to
79a6f4d
Compare
There was a problem hiding this comment.
I reviewed the latest push and found no new bugs; the second commit closes the separateSSRGraph: false gap in the client-component test. Because this changes which file every bake server/SSR import resolves to, with the trade-offs the description already lists, a human look at that design choice is still worthwhile.
What was reviewed:
- Everything else
BundleOptions::from_apiderives from the bun target besides main fields, extension order, loaders and env:tree_shaking/minify_syntaxare overwritten by bake,inliningis only read by the single-file transpile path (bundle_v2 takes it fromminify_syntax),import_path_formatis only used by the linker's single-file path,polyfill_node_globalsnow false for server/SSR is the right direction, andinit_external_modulestreats bun and browser identically. - The directory-watch retry:
bust_dir_cachestill runs only on the server resolver, but the dir cache andFileSystemare process singletons shared by the client resolver, so a client-graph retry sees the busted entry;Dep::default()'sGraph::Clientis only written into freed slots that are never read. Dep.graph == Ssrretries with the server resolver, whose conditions differ from the SSR resolver's only byreact-server; the retry only probesis_ok()and the real re-bundle re-resolves with the right graph, so this changes nothing in practice.
Extended reasoning...
The change moves the bake server/SSR transpilers from a browser-default Transpiler::init with post-hoc target patching to passing the bun target into init, restores env.behavior = disable when no env option is configured, resolves the framework's serverEntryPoint with the server resolver, and makes the dev server's failed-import retry use the importer graph's resolver via a new Dep.graph field; no security-sensitive surface is touched. Tests cover the dev server and bun build --app for the four package shapes, the .node extension lookup, the env non-inlining, and the browser-map retry. I did not approve because the PR knowingly changes resolution results for existing bake apps (a "use client" module's browser-only imports now fail the server-graph build; a package whose entry is a client component ships the server-resolved file to the browser), which is a product trade-off a maintainer should sign off on, and the earlier high-severity threads on those points were resolved by the author only.
Problem
"browser"map of a package. axios 1.6.8 fails there:There is no suitable adapter to dispatch the request.Framework::init_transpiler(src/runtime/bake/config/transpiler.rs:27) builds each transpiler for the browser, then assigns the target and the main fields. The extension order, the loaders and the resolver's"browser"map switch stay the browser's.Fix
init_transpilerpasses the target toTranspiler::init, which derives all four. Both assignments go. It sets environment inlining back to off.serverEntryPointresolves with the server resolver. The dev server retries a failed import with the importer graph's resolver.test/bake/dev(9 fail on the base). Self-reviewed: 39 concerns raised, 38 addressed. Not done: no issue filed.Background
"browser"map is a package.json object that replaces single files for a browser, or disables them withfalse.opts.targetfor every transpiler. That collides with bundler: derive the options of a server build's page from the browser target #46548 and is offered there.Downsides
"browser"map no longer disables a dependency: pdfjs-dist 3.11.174 fails the build withCould not resolve: "canvas"."use client"package entry reaches the client as the file the server resolved: nowmainwhere a"browser"map replaces it, as with anexportscondition..text: 256 bytes less.Notes
Stacked. The base of this PR is the branch of #46652, so the diff here is only the bake change. When that branch is rebased, this one is rebased onto it. When #46652 merges, this branch gets a rebase onto main.
What #46652 does for bake. It assigns the main-field list of the graph's target after
init. That corrects a package withmainand a string"browser", and one withmainandjsnext:main. Here the target goes intoinit, which derives that list with the rest, so the assignment goes.BundleOptions::main_fieldsispub(crate)again, as on main: no other crate assigns it.Repro (dev server, also
bun build --app). A framework liketest/bake/minimal.server.ts, then:fetch("/")prints200 ROUTE:BROWSER-SIDEon the base and200 ROUTE:SERVER-SIDEhere (release builds of both). With axios 1.6.8 in place ofdual-pkg, a route that callsaxios.get()fails on the base withThere is no suitable adapter to dispatch the request since : - adapter xhr is not supported by the environment - adapter http is not available in the build, and gets its response here. The"browser"map of axios replaces./lib/adapters/http.js.Nobody outside reported this.
appinBun.serveandbun build --appare gated (feature_flags::bake(): canary and debug builds, orBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE). #44496 lists "serverEntryPointis still resolved with the client's resolver" under what it left unchanged.What each graph resolves (debug builds of the base and of this branch, dev server and
bun build --app):main+"browser": "./browser.js"main+"browser": {"./main.js": "./browser.js"}main+"browser": {"./node.js": false}main+jsnext:mainjsnext:mainfileFor the third row the dev server of the base answers 500:
ReferenceError: import_node is not defined. That is the first defect under "Left as it is", reached in the server graph.Also new for a server graph:
require("../addon")findsaddon.node(base:Could not resolve: "../addon"), because a transpiler for bun has.nodein its extension order. Loading an addon in a server graph does not work, on the base too:import addon from "../addon.node"bundles with the napi loader on both and fails at the request withFailed to resolve dynamic import. The import without extension now fails there too.serverEntryPointgets the server's conditions (bun,node, notbrowser)."browser"map no longer redirects it.Why the target goes into
init.BundleOptions::from_apiandResolver::init1derive state from the target once: the main fields, the extension order, the loaders,env.behavior, and the resolver'scare_about_browser_field. bake assigned the target afterinitand rebuilt the conditions, and since #46652 the main fields. 9 of the 11 callers ofTranspiler::initalready put the target in the options. The other one that did not, thebun test --changedscan, is converted by #46652.The directory watcher. When an import fails, the dev server watches the directory and tries the import again on a change (
hot_reload_event.rs). It did that with the server resolver for every graph. That was harmless while the server resolver read"browser"maps. NowDepkeeps the graph of the importer, and the retry uses the client transpiler for the client graph. Without this, a client import that only a"browser"map can resolve never recovers (Reload not calledin the new test). This is the one part that reachesBun.serveHTML routes outside bake.Client components. A
"use client"file is bundled for the client graph and for the server. Each graph resolves the imports of its copy. The tests load the SSR copy throughssrManifest, as a framework does. Without an SSR graph they read the server graph's copy from the client reference.The server copy is code that runs. With
separateSSRGraph: trueit is the SSR copy, and the server graph has only a generated proxy (the first parse of the file uses the client resolver). WithseparateSSRGraph: falsethe server graph keeps the implementation insideregisterClientReference: the module is evaluated once on the server, andisland.value()returns what the component returns. So an import of the component that resolves only for a browser cannot be left out of either.pdfjs-dist 3.11.174 is such a dependency. Its
"browser"map has"canvas": false, itsbuild/pdf.jshasrequire("canvas"), andbun installdoes not install the optionalcanvas.bun build --appof a client component that imports it exits 0 on the base and 1 here:Could not resolve: "canvas". A client-only import behindimport.meta.env.SSRor a dynamic import avoids it. konva 8 and 9 (a string"browser", and amainthat needscanvas) already fail on the base, since it readsmain.A package whose entry file is itself a
"use client"module: every copy of a client component has the path that the importing resolver returned, so the client bundle gets what the server resolved.exportswith abrowserconditionmain+"browser": "./browser.js"main+"browser": {"./node.js": "./browser.js"}On the base the SSR copy of the last one is browser.js too. A fix resolves the import once per graph, and then one client component is two files: the boundary code in
bundle_v2.rs, the generated proxy and the dev server's graphs all pair the copies by one path today.Next.js, for comparison:
mainandmodule, and notbrowser(getMainFieldinpackages/next/src/build/webpack-config-rules/resolve.ts). konva fails there too:Module not found: Can't resolve 'canvas'(react-konva issues 750, 785 and 832).getModuleResourceinflight-client-entry-plugin.tsgives the reason: "to make sure the server and client are using the same module path (required by RSC), as the server compiler and client compiler have different resolve configs". Not tested: what the client compiler then does with a"browser"map for that path.Shape of this PR. The first version of this branch fixed the mechanism for every transpiler: the resolver read
opts.targetin place of the flag it computes once, and the projection to the resolver derived the default main fields and the polyfill switch. It also built the--changedscan for bun. A review found that #46652 carries the same scan change, that the resolver part decides what #46548 decides the other way (it keeps the flag and addsResolver::for_target), and that the two do not compile together. So this PR keeps what no other PR has. The resolver part is offered on #46548 with its numbers: each of the 6"browser"gates stays one byte compare and one branch, a warm resolve costs 0 to 3 more instructions, andTranspiler::initdoes 9 mallocs and 5 frees fewer.Left as it is.
"browser"map disables (ReferenceError: nodeFile is not defined). It is on main too, in the client graph. The client assertions here read the bundle text and do not run it."use client"module that a route imports and that abunBakeGraph: "ssr"module imports too makes the dev server panic in about 1 of 4 runs:Server Incremental Graph is missing component for "". It is on the base too. The tests avoid that shape.VirtualMachine::enable_macro_mode) and the page of a server build (initialize_client_transpiler) still assign the target. bundler: derive the options of a server build's page from the browser target #46548 owns the page.Measurements (release builds of #46652 at 0c2ef95 and of this change on it, linux-x64, with gdb and
size.perf,strace,valgrindandbloatyare not installed here. #46652 has since changed how it writes its main-field assignment. The numbers were not taken again for that)init_transpiler, server graph: mallocs / freesinit_transpiler, SSR graphinit_transpiler, client graph.envopens of a bake app with a.envfile (gdbcatch syscall).text(size -A)init_transpilerruns once per graph when a dev server starts orbun build --appruns. Outsidesrc/runtime/bakethe diff is one visibility keyword.Depgrows by one byte-sized field.The first version of this branch, on main without #46652, gave the client bundle
mainfor a shared string-"browser"package in 23 of 40 dev-server runs. With #46652 under it, both read orders are tests.Each line that sets something back or swaps a resolver has a test. Debug builds with one line removed, made at the earlier head d501d86 (these lines are the same now): without the
env.behaviorline, botha server route reads process.envtests fail. WithserverEntryPointon the client resolver, botha framework's server modules resolve for buntests fail. With the retry on the server resolver,importing a file that a "browser" map names before it is createdfails.Tests. 9 of the 12 new tests fail on a debug build of the base (#46652 at 0f1a0e9): the two read orders of
each graph resolves packages, the two modes ofa client component's packages, the framework test and its production twin, the.nodetest, the stylesheet test, and the production graph test. The other 3 pass on both and are the ones above.Suites (debug build of this branch at 79a6f4d): all of
test/bake, 26 files: 285 pass, 0 fail. One timing test of an HTML route,incremental graph handles edge deletion with next dependency, is not stable on this machine (Client sent 5 unread messages). Run alone 25 times, it fails 3 times on a build of the base and 4 times on a build of this branch. The React tests ofproduction.test.tsneed more than the default 5 s on a debug build here, with or without this change.