perf(dev): prune NODE_ENV during dependency optimization - #2075
Conversation
TheAlexLichter
changed the title
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the dep-optimizer NODE_ENV pruning change. The approach is sound and matches Next.js intent (forcing NODE_ENV during dev so React/RSC deps drop the unused branch), and it correctly respects a user-supplied process.env.NODE_ENV define. The Vite 7 (esbuildOptions) vs Vite 8 (rolldownOptions.transform) split is handled cleanly, and resolvedNodeEnv is mode-aware so the optimizer define stays consistent with the environment. Nice coverage with both the config-shape unit tests and the dev-server integration test that inspects the actual deps_rsc/deps_ssr output.
A few minor observations below — none blocking.
|
|
||
| export function serializeViteDefine(value: unknown): string { | ||
| if (typeof value === "string") return value; | ||
| return JSON.stringify(value) ?? "undefined"; |
There was a problem hiding this comment.
Minor: JSON.stringify(value) returns string | undefined, so the ?? "undefined" fallback covers value === undefined (which happens when the user sets define: { "process.env.NODE_ENV": undefined }, since Object.hasOwn is true in that case). Worth confirming this is the intended behavior — it produces the bare replacement text undefined rather than the string "undefined". That matches how Vite treats define values (raw replacement text), so it looks correct, but a one-line comment explaining the ?? "undefined" branch would help future readers since it's a non-obvious edge case.
| await response.arrayBuffer(); | ||
|
|
||
| for (const envName of ["rsc", "ssr"]) { | ||
| const depsDir = path.join(server.config.cacheDir, `deps_${envName}`); |
There was a problem hiding this comment.
This assertion reads from Vite's internal cache layout (deps_<env> under cacheDir). It's a strong, meaningful check today, but it couples the test to a Vite implementation detail that could shift across Vite versions. Consider adding a short comment noting the dependency on Vite's deps cache directory naming so a future Vite bump that renames these dirs surfaces an understandable failure rather than a confusing readdir ENOENT.
| ]), | ||
| ...(incomingInclude.length > 0 ? { include: incomingInclude } : {}), | ||
| rolldownOptions: { plugins: [depOptimizeAliasPlugin] }, | ||
| ...depOptimizeNodeEnvOptions, |
There was a problem hiding this comment.
Not a bug, just confirming intent: depOptimizeNodeEnvOptions is intentionally spread into the rsc/ssr/pages-server optimizeDeps but not into the client environment's optimizeDeps (client is a client consumer where Vite already replaces process.env.NODE_ENV by default). That's the right call. The top-level viteConfig.optimizeDeps here does get the define, and since it's also inherited by the client env in the non-app Pages path, the define is harmless there (client value is the same). Worth a one-liner noting that the define is deliberately scoped to server consumers, mirroring the helper's comment.
|
Review posted successfully on PR #2075. SummaryI reviewed PR #2075 ( Verdict: solid implementation, no bugs found. Key findings:
Minor observations raised:
|
|
Decoupled the test and added comments 👍 |
commit: |
2 participants
This PR improves cold start time by replacing
NODE_ENVduring Rolldown's prebundling step, resulting in less JavaScript dependencies during dev.Over 10x10 runs: