Repository navigation
fix: align Server Function proxy transforms with @vitejs/plugin-rsc - #4630
Conversation
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 7 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
james-elicx
changed the title
4d83a3e to
6fc8909
Compare
|
6fc8909 to
4284ac7
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
4284ac7 to
9e64e6e
Compare
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| use-count-up: 3.0.1 | ||
| validator: ^13.15.26 | ||
| vite: npm:@voidzero-dev/vite-plus-core@1.5.0 | ||
| vite: npm:@voidzero-dev/vite-plus-core@1.5.1 |
There was a problem hiding this comment.
This catalog bump fixes the workspace build, but it does not constrain consumers: packages/vinext/package.json still declares vite: ^8.0.0. Vite 8.3.6 depends on Rolldown ~1.2.16, whose empty-map bug retains the pre-transform sourcesContent (rolldown/rolldown#11176), so users on the currently supported Vite 8.0-8.3.6 range will still publish the bodies behind all three new proxies. Please raise the published Vite peer minimum to 8.3.7 (and account for the Vite+ alias if needed), or provide a compatibility path that actually removes the source on older Rolldown versions.
There was a problem hiding this comment.
Declining this one; the PR body's "For users" note documents the Vite ≥ 8.3.7 / Vite+ ≥ 1.5.1 requirement instead.
- Client sourcemaps are opt-in: vinext never enables
build.sourcemapfor the client (Next'sproductionBrowserSourceMapsalso defaults to off), so a peer minimum of 8.3.7 would force every vinext user to upgrade Vite for a setting most don't use. - Older Rolldown isn't a regression from this PR. Before it, these proxies returned
hires: "boundary"maps that carried the original module on every Rolldown version; the empty map is strictly no worse there. - plugin-rsc's own
"use server"change (fix(rsc): drop Server Function source from client sourcemaps vitejs/vite-plugin-react#1497) has the same Rolldown requirement, and plugin-rsc 0.5.38 keepsvite: "*". Raising only vinext's floor would diverge from upstream. - A compatibility path for old Rolldown means post-processing
sourcesContentingenerateBundle, which is exactly the workaround (fix(build): match Next.js sourcemap output for "use server" modules #3817) this PR replaces with the upstream fixes.
|
Review: 1 findings. Posted 1 actionable inline finding. No branch modifications made. |
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
e4af518e.
This review, of 9e64e6e2
💬 Approved with comments — The change is correct and has good end-to-end tests. The only notes are an optional unit test for the new helper and a JSDoc comment that should mention the Rolldown version requirement.
Findings: 1 info · 1 suggestion (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 suggestion
The change is correct. The new helper has no focused unit test.
Recommendation: 💬 Optional Nits
- [SUGGESTION] No unit test for the new
serverReferenceProxyResulthelper's build and dev branches (packages/vinext/src/plugins/transform-result.ts:47)
View full review
The three call sites (use-cache-callable.ts and the two worker transforms in worker-use-server.ts) use the new helper correctly. The { mappings: "" } type flows through the generic, and dev keeps the lazy full map.
The new serverReferenceProxyResult helper has no unit test of its own. At the moment only the browser e2e projects cover it, and those only fail on Rolldown ≥ 1.2.18. A couple of cases in tests/transform-result.test.ts would pin both branches in the fast Vitest suite.
📝 Documentation: 💬 1 info
The helper's JSDoc overstates the guarantee on older Rolldown.
Recommendation: 💬 Optional Nits
- [INFO] JSDoc doesn't mention that the empty-map guarantee needs Rolldown ≥ 1.2.18 (
packages/vinext/src/plugins/transform-result.ts:45)
View full review
The JSDoc on serverReferenceProxyResult says the empty map keeps the replaced module's source out of sourcemaps. Rolldown before 1.2.18 refills empty maps with the pre-transform source, as the description's own results table shows.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: low. The catalog bump to Vite+ 1.5.1 only changes the Vite+ packages and the postcss version they bring in.
🔒 Security: ✅ LGTM
No security issues. The change reduces how much server-only source can end up in client sourcemaps.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
Builds no longer generate maps for these proxies, and dev is unchanged.
Reviewed commit: 9e64e6e2
| /** | ||
| * Build the result of a transform that replaces a module with Server Function | ||
| * reference proxies. Like @vitejs/plugin-rsc's `rsc:use-server`, builds return | ||
| * an empty map so the replaced module's source doesn't ship in sourcemaps. |
There was a problem hiding this comment.
ℹ️ INFO · Documentation
JSDoc doesn't mention that the empty-map guarantee needs Rolldown ≥ 1.2.18
Rolldown before 1.2.18 (Vite < 8.3.7 / Vite+ < 1.5.1) refills an empty map's sourcesContent with the module's pre-transform code. On those versions the replaced module's source still ships in the sourcemap, which your own results table shows for Vite+ 1.5.0. A short note here, such as "requires Rolldown ≥ 1.2.18 to take effect", would stop future readers from assuming the guarantee holds on any Vite.
There was a problem hiding this comment.
Added in e4af518: the JSDoc now notes that Rolldown < 1.2.18 (Vite < 8.3.7) refills empty maps with the original code, so the helper needs Rolldown >= 1.2.18 to take effect.
| * reference proxies. Like @vitejs/plugin-rsc's `rsc:use-server`, builds return | ||
| * an empty map so the replaced module's source doesn't ship in sourcemaps. | ||
| */ | ||
| export function serverReferenceProxyResult<TMap = ReturnType<MagicString["generateMap"]>>( |
There was a problem hiding this comment.
💬 SUGGESTION · Code Quality
No unit test for the new serverReferenceProxyResult helper's build and dev branches
Only the browser-specific e2e projects cover this helper, and they only fail on Rolldown ≥ 1.2.18. A regression in the mode check or the dev fallthrough would get past the Vitest suite. Consider adding cases to tests/transform-result.test.ts. With { mode: "build" }, assert map equals { mappings: "" } and that generateMap is never called. With { mode: "dev" }, assert the result is a real map with the expected sources.
There was a problem hiding this comment.
Added in e4af518: tests/transform-result.test.ts now covers both branches. With { mode: "build" } the result is { mappings: "" } and generateMap is never called; with { mode: "dev" } the map is generated with the passed options (source included). 62 passed across transform-result, worker-use-server and use-cache-transform.
Build-mode Server Function proxies return an empty map, like plugin-rsc's "use server" proxies. Covers vinext's "use cache" client proxy and the Web Worker "use server" and "use cache" proxies. Upgrades Vite+ to 1.5.1 (Vite 8.3.7, Rolldown 1.2.18), and the app-action-ownership production e2e now builds with client sourcemaps. Pages Router tests decode the html-proxy script src, which Vite now HTML-escapes (`&`).
9e64e6e to
e4af518
Compare
|
clanky review |
|
/bigbonk review for issues Please review exact head |
There was a problem hiding this comment.
✅ Approved — Both earlier notes, the missing unit test and the Rolldown version in the JSDoc, are fixed at e4af518, and no new issues were found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The catalog bump to Vite+ 1.5.1 only changes the Vite+ packages and the postcss version they bring in.
🔒 Security: ✅ LGTM
No security issues. The change reduces how much server-only source can end up in client sourcemaps.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
tests/transform-result.test.ts now tests the build branch of serverReferenceProxyResult (an empty map, with generateMap never called) and the dev branch (the map is generated with the options passed in). All three call sites pass this.environment to the helper, and the worker unit test now gives its hook context environment: { mode: "build" }.
⚡ Performance: ✅ LGTM
Builds no longer generate maps for these proxy transforms. Dev still generates the map lazily, as before.
📝 Documentation: ✅ LGTM
The JSDoc for serverReferenceProxyResult now says that Rolldown < 1.2.18 refills empty maps with the module's original code.
Reviewed commit: e4af518e
|
LGTM! |
Summary
Upgrades Vite+ to 1.5.1 and aligns vinext's Server Function proxy transforms with
@vitejs/plugin-rsc. In builds, transforms that replace a module withcreateServerReference()proxies now return an empty sourcemap, so client sourcemaps (environments.client.build.sourcemap) only map the code the browser actually receives.Two upstream releases handle plugin-rsc's own
"use server"proxies:@vitejs/plugin-rsc0.5.37 (fix(rsc): drop Server Function source from client sourcemaps vitejs/vite-plugin-react#1497) returns{ mappings: "" }for its"use server"proxies in builds. vinext's catalog already has^0.5.38.{ mappings: '' }) like Rollup rolldown/rolldown#11176) stops keepingsourcesContentfor sources that no mapping reaches. Before that, rolldown refilled an empty map with the module's pre-transform code, so the plugin-rsc change had no effect. Vite 8.3.7 is the first Vite on it, and Vite+ 1.5.1 bundles Vite 8.3.7 and rolldown 1.2.18 (chore(deps): upgrade Vite, Rolldown, and PostCSS voidzero-dev/vite-plus#3390).vinext has three Server Function proxy transforms of its own that the upstream fixes don't cover:
"use cache"modules imported by Client Components;"use server"modules imported by Web Workers;"use cache"modules imported by Web Workers.These kept
generateMap({ hires: "boundary" }), which maps the proxy back to the original module, so rolldown 1.2.18 rightly keeps that module's content. This PR gives all three the same treatment as plugin-rsc: an empty map in builds, through a sharedserverReferenceProxyResult()inplugins/transform-result.ts. Dev keeps the full map.Vite+ 1.5.1
vite→npm:@voidzero-dev/vite-plus-core@1.5.1andvite-plus→1.5.1in the catalog, like #4602. The lockfile diff is only the Vite+ packages pluspostcss8.5.31 → 8.5.32 (published 2026-10-26), which vite-plus-core depends on.Vite+ 1.5.1 was published today, so the lockfile was resolved once with
--config.minimum-release-age=0. Workspace config is unchanged and nominimumReleaseAgeExcludeis needed: frozen-lockfile installs don't apply the release-age check.Vite 8.3.6 (vitejs/vite#23995) HTML-escapes attribute values, so the Pages Router dev hydration script is now served as
src="…?html-proxy&index=0.js". Browsers decode that to&, buttests/pages-router.test.tsscraped the raw attribute and fetched a 404, so its html-proxy lookups now go through agetHtmlProxyScriptPath()helper that decodes&. vinext's own HTML handling is unaffected.For users: projects on Vite+ < 1.5.1 or Vite < 8.3.7 still need to upgrade for plugin-rsc's
"use server"proxies. vinext's own proxies are fixed on any Vite.Supersedes #3817
This supersedes #3817 (draft). That PR scrubbed
sourcesContentingenerateBundlewith a ~265-line plugin. With the upstream fixes, no scrubbing is needed. I left #3817 open for you to close.mainhas no other code that post-processes client sourcemaps for Server Functions. The onlysourcesContentreader is the dev stack-frame code inserver/dev-stack-sourcemap.ts, which is unrelated.Tests
The tests are in the existing
app-action-ownershipproduction suite (tests/e2e/app-router/action-owner-production.browser.spec.ts), which runs in the Chrome and WebKit browser-specific projects. Its build now enablesenvironments.client.build.sourcemap, like Next's actions fixture enablesproductionBrowserSourceMaps, so every ownership test runs against a sourcemapped build.A new route,
/ownership/sourcemap, adds:"use server"module imported by a Client Component;"use server"action;"use cache"module imported by the same Client Component;"use cache"function;Each body has a unique marker. Two new tests assert:
.jsor.js.mapunderdist/clientcontainsuse server,use cacheor any marker. This also covers the fixture's existing client-imported actions;client-buttons.tsxandworker.tswith their content, so the test fails if sourcemaps silently stop being emitted;sourceMappingURLreturns 200 and contains none of the server-only text.The new route adds two
"use cache"references, sofindOnlyCacheActionIdbecamefindCacheActionId(source, "report/cache-owner/actions.ts:cachedProtectedAction"). It matches the reference id registered for that export.The two new tests on each combination (Chrome):
use serverin 6 other chunks (the fixture's existing client-imported actions), plus the new route's chunk and worker mapcached.ts; worker map: both modulesAlso run locally on the final head:
action-owner-production.browser.spec.ts: 29 passed in Chrome and 29 in WebKit;vp test run tests/transform-result.test.ts tests/worker-use-server.test.ts tests/use-cache-transform.test.ts: 60 passed. The worker test's hook context now passesenvironment: { mode: "build" };vp test run tests/pages-router.test.ts: 498 passed (5 failed on Vite+ 1.5.1 before the helper);vp check,vp run knip: clean.Earlier, with the same vinext changes: e2e
web-worker-vinext1 passed;app-router-isr-produse-cache.spec.ts+action-bound-args.spec.ts27 passed.Performance
Builds now skip generating these proxies' maps; dev is unchanged. With the vinext changes alone, the
tests/fixtures/app-basicproduction build median was main 9871 ms vs branch 9598 ms (3 alternating runs, within noise). The CI perf comparison on the final head (9e64e6e vs base): 0 improved, 0 regressed, 7 within ±1.5% — production build 2.65 s → 2.67 s, dev cold start 2.17 s → 2.14 s, large Pages Router dev cold start 17.96 s → 17.89 s, bundle sizes unchanged.