fix(build): set correct resolve.conditions per Vite environment#1396
fix(build): set correct resolve.conditions per Vite environment#1396james-elicx wants to merge 1 commit into
Conversation
Package `exports` maps were resolving to the wrong file in each Vite environment because vinext never set `resolve.conditions` on its environments — Vite fell back to its defaults, which lack `react-server` on RSC and `edge-light`/`worker` on workerd-targeting environments. Set explicit conditions per environment, mirroring Next.js webpack: - RSC env: `react-server` + server defaults (+ edge conditions on workerd) - SSR env: server defaults (`module`, `node`, ...) (+ edge conditions on workerd) - Client env: client defaults (`module`, `browser`, ...) Closes #1356.
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(build): set correct resolve.conditions per Vite environment
Reviewed the implementation and test coverage. This is a clean, well-targeted fix for a real resolution gap.
What's correct
The core approach is right: since Vite's resolve.conditions for environment configs does full replacement (not merge), explicitly setting the conditions per environment is the only way to ensure the correct export map entries are resolved. The PR correctly uses Vite's "development|production" sentinel value (matching defaultClientConditions / defaultServerConditions), which Vite internally resolves to the appropriate condition based on build mode.
The condition lists match Next.js semantics:
- RSC:
react-server+ server defaults (and edge conditions when targeting workerd) - SSR: server defaults (and edge conditions when targeting workerd)
- Client: browser defaults
The restructuring of the resolve object — pulling conditions up to the resolve level while spreading external/noExternal conditionally — preserves the existing behavior while adding the new conditions cleanly.
Test quality
The tests are well-structured: each test creates a fresh temp directory with proper app/ or pages/ structure, creates a fresh vinext() plugin instance (avoiding shared state), and properly cleans up. The NODE_ENV save/restore in beforeEach/afterEach handles the config hook's side effect correctly. Good coverage of all five environment × platform combinations.
Observations (non-blocking)
-
Pages Router on Cloudflare (line 1875): This branch only defines a
clientenvironment — nossrenvironment is emitted. The SSR for Pages Router on Cloudflare runs in the Cloudflare plugin's own worker environment, which sets its ownedge-light/workerd/workerconditions. So the omission is correct, but a brief comment explaining why there's no SSR env here (and thus no SSR conditions to set) would help future readers avoid the same "wait, where are the SSR edge conditions?" question. -
nodecondition on edge SSR: WhenhasCloudflarePluginis true, the SSR conditions include bothnodeandedge-light/workerd/worker. Thenodecondition comes fromSERVER_DEFAULT_CONDITIONS. This matches Vite's defaults but is technically inaccurate for workerd (which is not a Node runtime). In practice this shouldn't cause issues — packages rarely have bothnodeandedge-lightexports that conflict — but it's worth noting as a potential future refinement. Next.js uses differentconditionNamesarrays for Node vs Edge runtimes and does not includenodewhen targeting edge. -
No test for Pages Router on Cloudflare client conditions: The Pages Router tests cover
pages/on Node (checking SSRnodeand clientbrowser), but there's no test for Pages Router + Cloudflare plugin. The implementation does set client conditions in that branch (line 1887), so a test would be nice for completeness — though the existing App Router + Cloudflare tests provide good confidence the detection logic works.
Looks good to merge.
|
Review posted and approved on PR #1396. Here's a summary: Approved. The PR correctly fixes the missing
I noted three non-blocking observations:
|
1 participant
Summary
Package
exportsmaps were resolving to the wrong file in each Vite environment because vinext never setresolve.conditionson its environments — Vite fell back to its defaults, which lackreact-serveron RSC andedge-light/workeron workerd-targeting environments.Closes #1356.
This is the equivalent of Next.js' webpack
conditionNamesconfiguration (seereactServerConditionNamesand Next.js'test/e2e/import-conditions/).What changed
Explicit
resolve.conditionsper Vite environment:react-server+ server defaults (module,node, dev/prod). On workerd targets (Cloudflare / Nitro), alsoedge-light,workerd,worker.edge-light,workerd,worker.module,browser, dev/prod).node/browserconditions.@vitejs/plugin-rscalready setsreact-serveron its own rsc env config; setting it here makes the behaviour explicit and order-independent, and adds the edge runtime conditions that no other plugin contributes for vinext's rsc/ssr envs.Test plan
tests/import-conditions.test.tscovering RSC / SSR / client conditions for App Router and Pages Router, both with and without the Cloudflare plugin (7 tests, all 7 failed onmainand pass on this branch).pnpm test tests/import-conditions.test.tspnpm test tests/build-optimization.test.ts— 75 passing (no regressions in env config)pnpm test tests/optimize-imports.test.ts tests/app-router.test.ts tests/pages-router.test.ts— 568 passingpnpm run check— clean