Repository navigation
fix(optimize-imports): only stop at use client modules in RSC - #4102
Conversation
#4099 made every directive module the import source for the names it exports, in both server environments. Record the boundary per environment instead: RSC stops at any directive, while SSR only stops at use server and use cache modules, which plugin-rsc and vinext replace with references there. SSR again rewrites imports past use client barrels such as antd's entry. Also skip the rewrite for any package whose entry defines every imported name, not just react-aria-components subpaths, so imports from a use client entry keep their bare specifier in RSC.
|
/bigbonk review this PR. You have an 8-minute time limit. Head SHA: d9c759f |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
|
LGTM! |
Performance benchmarksCompared 0 improved · 0 regressed · 7 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
|
/bigbonk review this PR. You have an 8-minute time limit. Head SHA: 95ddd43 |
|
LGTM! |
|
/bigbonk review this PR. You have an 8-minute time limit. Head SHA: e2df5ea |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| const isSsrBoundary = boundaryDirective !== "use client"; | ||
| for (const [name, entry] of exportMap) { | ||
| exportMap.set(name, { | ||
| ...entry, |
There was a problem hiding this comment.
Preserving the leaf source here means the registration loop at lines 903-913 now registers bare dependencies hidden behind a directive boundary, even when this environment's effective source is the boundary module. In RSC, for example, a "use client" barrel that re-exports from a nested package causes resolveId to redirect unrelated direct imports of that package to the barrel's private copy (subpkgOrigin is global per environment and first-wins). Before this change the boundary replaced source, so that stale registration did not occur. Build this registration from the environment-effective entries (getEntry) so only sources this environment can actually emit are installed.
There was a problem hiding this comment.
Fixed in f5acbef: the registration loop now uses this environment's sources (getEntry), so a package behind a boundary isn't registered in RSC. Added a test that fails without it.
| ) { | ||
| // An entry that defines (or is the boundary for) every imported name | ||
| // gains nothing from a rewrite. | ||
| if (specifiers.every(({ imported }) => getEntry(imported)?.source === barrelEntry)) { |
There was a problem hiding this comment.
This broader no-op guard also skips the absolute rewrite when the package entry itself is a "use server"/"use cache" boundary. In Nitro dev, bare server imports outside plugin-rsc's noExternal package list are externalized before resolveId/transform runs (index.ts documents this at lines 4110-4117), so such an optimized package can expose its raw implementation in SSR instead of a server reference. Keep the rewrite for server boundaries (or otherwise force configured optimized packages through Vite); only a client boundary in RSC is safe to leave bare for the case added here.
There was a problem hiding this comment.
Fixed in f5acbef: the no-op guard is back to subpath imports only, so a bare import from a boundary entry is rewritten to its absolute path again in both environments. The "use client" entry test now expects that rewrite in RSC.
|
Review: 2 findings. Posted 2 actionable inline warnings. |
|
/bigbonk review this PR. You have an 8-minute time limit. Head SHA: f5acbef |
| for (const name of exportMap.keys()) { | ||
| // Only the sources this environment can emit, so a package behind | ||
| // a boundary isn't registered. | ||
| const source = getEntry(name)!.source; |
There was a problem hiding this comment.
The per-environment filtering here is still bypassed by the resolveId cross-environment fallback at lines 785-786. After SSR transforms this use-client barrel it registers the bare leaf source here; a later RSC resolution misses its own map, reads that SSR entry, and resolves the package from the barrel location, potentially selecting the private nested copy instead of the app copy. On main, the shared boundary source prevented SSR from registering this leaf, so this is introduced by the split maps. Please prevent fallback entries that this environment cannot emit (or remove the cross-environment fallback), and cover the SSR-transform-then-RSC-resolve sequence.
There was a problem hiding this comment.
Fixed in f7be786: resolveId now looks up the map keyed like the transform (rsc or ssr) and no longer falls back to the other environment's. Every specifier an environment emits is registered in its own map first, so the fallback only ever served specifiers that environment didn't emit. The test now runs the SSR transform before the RSC one and checks that only SSR resolves from the barrel.
|
Review: 1 finding. Posted 1 actionable inline warning. |
|
/bigbonk review this PR. You have an 8-minute time limit. Head SHA: f7be786 |
| // Sub-package specifiers are keyed per environment so that RSC and SSR | ||
| // barrel entries don't cross-contaminate each other's resolution context. | ||
| envOriginMap.set(entry.source, barrelEntry); | ||
| envOriginMap.set(source, barrelEntry); |
There was a problem hiding this comment.
In SSR, this newly registers bare leaves behind a "use client" barrel, but the map is still global by source within that environment and resolveId does not inspect the importer. A module that imports { Slot } from an optimized barrel with a nested @radix-ui/react-slot and also imports @radix-ui/react-slot directly will therefore resolve both imports from the barrel location; likewise, the first of two barrels with different nested versions wins. On main the client boundary prevented this SSR registration, so this is newly reachable here. Resolve the emitted leaf to an absolute id during the transform, or scope the origin mapping to the transformed importer/edge, so unrelated SSR imports retain their own package resolution.
There was a problem hiding this comment.
Correction after testing it on a fixture: this can't happen as described. Vite's vite:resolve runs before this plugin's resolveId, so any bare import the importing module can resolve, including a direct import of @radix-ui/react-slot, is resolved from the importer and never reaches subpkgOrigin. The map only handles specifiers the importer can't resolve, which is the pnpm strict case it exists for. I checked with a barrel that re-exports its own copy of a package the app also depends on: the app's direct import got the app's copy. That leaves two optimized barrels that both re-export different versions of a package the app can't resolve itself, which applies on main too and isn't specific to this PR. Not changing this here.
|
Review: 1 finding. Posted 1 actionable inline warning. |
|
/bigbonk review this PR. You have an 8-minute time limit. Head SHA: f7be786 |
|
LGTM! |
|
✅ Approved — No issues found. RSC and SSR now track their boundaries separately, which keeps the antd-style The decision is advisory: this comment never approves, blocks or requests changes on the pull request. ✅ All checks passed🔒 Security: ✅ LGTMThe plugin change only affects which module an import is rewritten to in the Vite dev and build pipeline. It handles no untrusted input, authentication, secrets or injection-prone data. The new fixture package is local and comes in as a 📖 AGENTS.md: ✅ LGTMThis pull request does not change any agent instruction files. The change follows the repository guidelines: it uses 📋 Code Quality: ✅ LGTMI checked these parts of the change against the repository:
⚡ Performance: ✅ LGTMNo performance concerns. The change brings back leaf-module rewrites in SSR for 📝 Documentation: ✅ LGTMThe new comments on Reviewed commit: |
1 participant
Follow-up to #4099, from the review notes on that PR.
Summary
#4099 stopped
vinext:optimize-importsfrom following re-exports through modules with a"use client","use server"or"use cache"directive, by making such a module the import source for every name it exports. The export map is shared between therscandssrenvironments, so this also applied in SSR, where"use client"has no effect.antd 6 is on the default list, and its entry (
es/index.js) starts with"use client". Since #4099, a server-sideimport { Button } from "antd":es/button/index.js, so dev loads the whole barrel again, which the default list is meant to avoid.noExternallist beforeresolveIdruns).Changes:
"use server"and"use cache", since plugin-rsc and vinext replace those modules with references in SSR too. One cache still serves both environments.resolveIdcome from this environment's sources too, so a package hidden behind a boundary isn't registered in RSC.resolveIdalso stops falling back to the other environment's registrations, which would otherwise bring SSR's back into RSC. Each environment registers every specifier it emits before emitting it, so the fallback only ever picked up specifiers this environment didn't emit.${"/app"}interpolation intests/optimize-imports.test.ts.Testing
Unit tests in
tests/optimize-imports.test.ts:export *, including the outermost boundary winning."use client"entry are rewritten to the entry in RSC and to the component module in SSR."use server"modules stay the import source in both RSC and SSR."use client"entry is resolved from the barrel in SSR but not in RSC, even after SSR has transformed the import.The first two fail without this change. The third passes on
mainas well, and guards SSR's"use server"handling. The fourth covers registering leaf sources, which this change now preserves.E2E in
tests/e2e/app-router/optimize-imports-client-barrel.spec.ts, onapp-basicin dev:fake-client-barrelis a localfile:package (__test_packages__/fake-client-barrel), added tooptimizePackageImports. Its entry is a"use client"barrel, and its unused leaf sets a global when it is evaluated."use client"page imports one component from it. The SSR HTML must show that the unused leaf never ran, and the component must hydrate and update.The SSR check fails without this change; the hydration check passes either way. The
pnpm-lock.yamlchange only adds the fixture'sfile:dependency.