fix(vite-8): remove deprecated Rollup config options for Vite 8/Rolldown#548
fix(vite-8): remove deprecated Rollup config options for Vite 8/Rolldown#548yunus25jmi1 wants to merge 3 commits intocloudflare:mainfrom
Conversation
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: Vite 8 / Rolldown compat
The overall approach of version-gating the Rollup-specific options is correct and the right path forward for Vite 8 support. However, there are several issues with the current implementation that should be addressed before merging.
1. Duplicate version detection in cli.ts (bug risk)
The CLI introduces its own viteMajorVersion by parsing the getViteVersion() string, but index.ts already has a robust getViteMajorVersion() function that resolves from process.cwd() and falls back to 7. The CLI's approach (parseInt(getViteVersion().split(".")[0], 10)) will produce NaN if getViteVersion() returns "unknown" (which happens before loadVite() is called, though in this case it's called after). Two different version detection mechanisms for the same thing is a maintenance liability.
The getViteMajorVersion() from index.ts should be exported and reused in the CLI instead of re-deriving it.
2. Missing tests for the new functions
The existing test file tests/build-optimization.test.ts has tests that assert the exact shape of the config (e.g., expect(result.build.rollupOptions.treeshake).toEqual({ preset: "recommended", moduleSideEffects: "no-external" })). These tests pass today because CI runs on Vite 7, where the old config is returned. But there are no tests for the Vite 8 path — the viteVersion >= 8 branches are completely untested. At minimum, add unit tests for getClientOutputConfig() and getClientTreeshakeConfig() that verify both the Vite 7 and Vite 8 return values.
3. Breaking public export change
The CLI import changed from clientOutputConfig/clientTreeshakeConfig to getClientOutputConfig/getClientTreeshakeConfig, but the original constants are still exported from index.ts. This means any external consumer importing the constants will still get the Rollup-specific values. The new getter functions are also exported, which is fine, but the PR description says nothing about the backwards-compatibility story. Since the old constants are still exported and the new getters are additive, this is actually fine — but the CLI should arguably still be importing the constants for Vite 7 (or just use the getters consistently). No action needed, just noting for clarity.
4. treeshake in rollupOptions for Vite 8
According to the issue description and Vite 8 migration docs, treeshake config for Rolldown should go under build.rolldownOptions.treeshake, not build.rollupOptions.treeshake. This PR strips the invalid preset key but still passes { moduleSideEffects: "no-external" } under rollupOptions.treeshake. If Rolldown ignores rollupOptions.treeshake entirely, this config has no effect. The PR should verify whether moduleSideEffects under rollupOptions.treeshake actually works in Vite 8, or whether it needs to move to rolldownOptions.treeshake.
packages/vinext/src/cli.ts
Outdated
| console.log(`\n vinext build (Vite ${getViteVersion()})\n`); | ||
|
|
||
| const isApp = hasAppDir(); | ||
| const viteMajorVersion = parseInt(getViteVersion().split(".")[0], 10); |
There was a problem hiding this comment.
This re-derives the Vite major version by parsing the version string, but index.ts already has a getViteMajorVersion() function that does this robustly (with createRequire from cwd and a fallback to 7). If getViteVersion() returns "unknown" (its default before loadVite() resolves), parseInt("unknown".split(".")[0], 10) returns NaN, which would cause both getter functions to return the Vite 7 config (since NaN >= 8 is false) — so it happens to work, but accidentally.
Consider exporting getViteMajorVersion from index.ts and using it here instead:
| const viteMajorVersion = parseInt(getViteVersion().split(".")[0], 10); | |
| const viteMajorVersion = getViteMajorVersion(); |
(This would also require adding getViteMajorVersion to the import from ./index.js.)
packages/vinext/src/index.ts
Outdated
| * Get Rollup-compatible output config for client builds. | ||
| * Returns config without Vite 8/Rolldown-incompatible options. | ||
| */ | ||
| function getClientOutputConfig(viteVersion: number) { |
There was a problem hiding this comment.
The return type is implicitly { manualChunks: ... } for Vite 8 and { manualChunks: ..., experimentalMinChunkSize: number } for Vite 7. Consider adding an explicit return type annotation so callers know what to expect, and to protect against accidental regressions:
function getClientOutputConfig(viteVersion: number): typeof clientOutputConfig | { manualChunks: typeof clientManualChunks } {| * Returns config without Vite 8/Rolldown-incompatible options. | ||
| */ | ||
| function getClientTreeshakeConfig(viteVersion: number) { | ||
| if (viteVersion >= 8) { |
There was a problem hiding this comment.
Same feedback as getClientOutputConfig — the Vite 8 branch strips preset and returns { moduleSideEffects: "no-external" } under rollupOptions.treeshake. However, the issue (#540) and Vite 8 migration docs suggest that Rolldown's treeshake config should be under build.rolldownOptions.treeshake, not build.rollupOptions.treeshake. If Vite 8/Rolldown ignores rollupOptions.treeshake entirely, then moduleSideEffects: "no-external" has no effect and the optimization is silently lost.
Please verify that rollupOptions.treeshake.moduleSideEffects is actually respected by Vite 8/Rolldown. If not, the Vite 8 path may need to return the config under a different key, or the caller sites need to conditionally place it under rolldownOptions instead.
|
Review submitted successfully. Here's a summary of the feedback I posted on PR #548: Requested changes with 4 key issues:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Vite 8 / Rolldown compat — version-gated config
The approach is sound: version-gate Rollup-specific config options so they're only applied on Vite 7, and strip the unsupported keys for Vite 8+. The implementation is clean, tests cover both branches, and the CLI correctly reuses getViteMajorVersion() from index.ts.
I see the previous bot review flagged "duplicate version detection in cli.ts" — that appears to have been fixed in the second commit. The current code imports and calls getViteMajorVersion() directly. That concern is resolved.
Two substantive items remain:
1. rollupOptions.treeshake vs rolldownOptions.treeshake for Vite 8
This is the most important open question. Issue #540 and the Vite 8 migration docs both say treeshake config should go under build.rolldownOptions.treeshake for Rolldown. This PR strips the invalid preset key but still passes { moduleSideEffects: "no-external" } under rollupOptions.treeshake.
If Vite 8 / Rolldown silently ignores rollupOptions.treeshake entirely, then moduleSideEffects: "no-external" has no effect and the barrel-library DCE optimization (the whole reason this config exists) is silently lost. The warnings go away, but the optimization goes with them.
Before merging, this should be verified with an actual Vite 8 build. If Rolldown ignores it, the Vite 8 path needs to place the treeshake config under rolldownOptions instead (or at the build.treeshake top level, depending on what Vite 8 actually supports). This would affect both the getter function return value and the call sites that spread it into the config.
2. Return type annotations are overly complex
The union return types on both getters use typeof references that make the signature harder to read than necessary. Since these functions are exported for CLI and testing, simpler types would be friendlier. This is a minor style nit, not a blocker.
| // Vite 8+ uses Rolldown which doesn't support `preset` option | ||
| // moduleSideEffects is still supported in Rolldown | ||
| return { | ||
| moduleSideEffects: "no-external" as const, |
There was a problem hiding this comment.
This is the critical question for the whole PR: does Vite 8 / Rolldown actually respect rollupOptions.treeshake.moduleSideEffects?
The issue (#540) and the Vite 8 migration docs both indicate that Rolldown treeshake config should go under build.rolldownOptions.treeshake, not build.rollupOptions.treeshake. If Rolldown ignores rollupOptions.treeshake entirely, this returns a config object that has no effect — the warnings are gone but so is the optimization.
Could you verify this with an actual Vite 8 build? If rollupOptions.treeshake is ignored, the Vite 8 path may need to return the config under a different key, or the call sites in the plugin/CLI need to conditionally place it under rolldownOptions.
packages/vinext/src/index.ts
Outdated
| */ | ||
| function getClientOutputConfig( | ||
| viteVersion: number, | ||
| ): typeof clientOutputConfig | { manualChunks: typeof clientManualChunks } { |
There was a problem hiding this comment.
Minor: the union return type is a bit unwieldy. Since this is an exported API, a simpler signature would be easier for consumers to work with:
| ): typeof clientOutputConfig | { manualChunks: typeof clientManualChunks } { | |
| function getClientOutputConfig( | |
| viteVersion: number, | |
| ): { manualChunks: typeof clientManualChunks; experimentalMinChunkSize?: number } { |
Same applies to getClientTreeshakeConfig below. Not a blocker — the current types are correct, just verbose.
| clientTreeshakeConfig, | ||
| computeLazyChunks, | ||
| getClientOutputConfig, | ||
| getClientTreeshakeConfig, |
There was a problem hiding this comment.
Good that both the old constants and the new getters are exported — preserves backwards compatibility for any external consumers importing clientOutputConfig / clientTreeshakeConfig directly. Worth adding a brief @deprecated JSDoc on the old constants so consumers know to migrate to the getters:
/** @deprecated Use `getClientOutputConfig()` instead — applies version-gated config. */
const clientOutputConfig = { ... };
|
Review posted successfully. Here's a summary of what I flagged: Key concern: Minor items:
What looks good:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Vite 8 / Rolldown version-gated config
The intent is correct — version-gating Rollup-specific config options so Vite 8/Rolldown doesn't choke on preset and experimentalMinChunkSize. The getter functions, tests for both branches, @deprecated JSDoc, and CLI reusing getViteMajorVersion() are all good.
However, there are several issues with the current implementation, ranging from a real correctness bug to significant code duplication.
Issues
1. onwarn in Vite 8 branch reads from rollupOptions instead of rolldownOptions (bug)
In the Vite 8 branch of the config hook, the onwarn closure captures config.build?.rollupOptions?.onwarn to preserve user-supplied warning handling. But if a user on Vite 8 puts their onwarn in rolldownOptions (the correct location for Vite 8), it won't be found. This is a latent bug — the user's onwarn is silently dropped.
2. Multi-environment builds don't get the rollupOptions → rolldownOptions treatment
The multi-env paths (App Router client environment at line ~1584, Pages Router Cloudflare client at line ~1601) use getClientBuildOptionsWithInput() which correctly returns rolldownOptions for Vite 8. But the existing integration test at line 436 still asserts result.environments.client.build.rollupOptions.treeshake with Vite 7 values. On Vite 8, the config would be under rolldownOptions, and this test would fail. The test isn't wrong today (CI runs Vite 7), but it means the multi-env Vite 8 path is completely untested.
3. Massive onwarn duplication in the config hook
The config hook now has three near-identical copies of the onwarn closure: the Vite 8 rolldownOptions branch, the Vite 7 rollupOptions branch, and (implicitly) the old code path. The ternary nesting is already hard to follow; duplicating the 15-line onwarn factory makes it worse. This should be extracted into a helper function and referenced from both branches.
4. as any cast on the Vite 8 build config
Line 1353 casts the entire Vite 8 build config as any to work around Vite 7 types not including rolldownOptions. This silences all type checking for that branch. A more targeted approach would be to use a type assertion only on the rolldownOptions key, or to use Vite 8 types when available.
5. getClientBuildOptions and getClientBuildOptionsWithInput are near-identical
These two functions share ~95% of their logic and differ only in whether input is included. getClientBuildOptionsWithInput could simply call getClientBuildOptions and spread input into the result, or getClientBuildOptions could accept an optional input parameter. The current approach doubles the maintenance surface for the type signatures and branching logic.
| // final bundle. We preserve any user-supplied onwarn so custom | ||
| // warning handling is not lost. | ||
| onwarn: (() => { | ||
| const userOnwarn = config.build?.rollupOptions?.onwarn; |
There was a problem hiding this comment.
Bug: In the Vite 8 branch, this reads config.build?.rollupOptions?.onwarn to preserve user-supplied warning handling. But a Vite 8 user would configure onwarn under rolldownOptions, not rollupOptions. This silently drops the user's onwarn.
This should read from rolldownOptions in the Vite 8 branch:
| const userOnwarn = config.build?.rollupOptions?.onwarn; | |
| const userOnwarn = config.build?.rolldownOptions?.onwarn ?? config.build?.rollupOptions?.onwarn; |
(Checking both covers the transition period where users might still have rollupOptions config.)
| }; | ||
| })(), | ||
| }, | ||
| } as any, |
There was a problem hiding this comment.
The as any cast here disables all type checking for the entire Vite 8 build config object. This means if you misspell a property name or pass the wrong shape, the compiler won't catch it.
Consider a narrower cast, e.g.:
build: {
["rolldownOptions" as string]: { ... },
}Or define a minimal type for the Vite 8 config shape.
| : viteMajorVersion >= 8 | ||
| ? { | ||
| build: { | ||
| rolldownOptions: { | ||
| ...getClientBuildOptions(viteMajorVersion).rolldownOptions, | ||
| // Suppress "Module level directives cause errors when bundled" | ||
| // warnings for "use client" / "use server" directives. Our shims | ||
| // and third-party libraries legitimately use these directives; | ||
| // they are handled by the RSC plugin and are harmless in the | ||
| // final bundle. We preserve any user-supplied onwarn so custom | ||
| // warning handling is not lost. | ||
| onwarn: (() => { | ||
| const userOnwarn = config.build?.rollupOptions?.onwarn; | ||
| return (warning: any, defaultHandler: (warning: any) => void) => { | ||
| if ( | ||
| warning.code === "MODULE_LEVEL_DIRECTIVE" && | ||
| (warning.message?.includes('"use client"') || | ||
| warning.message?.includes('"use server"')) | ||
| ) { | ||
| return; | ||
| } | ||
| if (userOnwarn) { | ||
| userOnwarn(warning, defaultHandler); | ||
| } else { | ||
| defaultHandler(warning); | ||
| } | ||
| }; | ||
| })(), | ||
| }, | ||
| } as any, | ||
| } | ||
| : { | ||
| build: { | ||
| rollupOptions: { | ||
| ...getClientBuildOptions(viteMajorVersion).rollupOptions, | ||
| // Suppress "Module level directives cause errors when bundled" | ||
| // warnings for "use client" / "use server" directives. Our shims | ||
| // and third-party libraries legitimately use these directives; | ||
| // they are handled by the RSC plugin and are harmless in the | ||
| // final bundle. We preserve any user-supplied onwarn so custom | ||
| // warning handling is not lost. | ||
| onwarn: (() => { | ||
| const userOnwarn = config.build?.rollupOptions?.onwarn; | ||
| return (warning: any, defaultHandler: (warning: any) => void) => { | ||
| if ( | ||
| warning.code === "MODULE_LEVEL_DIRECTIVE" && | ||
| (warning.message?.includes('"use client"') || | ||
| warning.message?.includes('"use server"')) | ||
| ) { | ||
| return; | ||
| } | ||
| if (userOnwarn) { | ||
| userOnwarn(warning, defaultHandler); | ||
| } else { | ||
| defaultHandler(warning); | ||
| } | ||
| }; | ||
| })(), | ||
| }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
The onwarn factory is duplicated verbatim across the Vite 8 and Vite 7 branches (15 lines each). Extract it into a helper:
function createDirectiveOnwarn(userOnwarn?: (warning: any, handler: (w: any) => void) => void) {
return (warning: any, defaultHandler: (warning: any) => void) => {
if (
warning.code === "MODULE_LEVEL_DIRECTIVE" &&
(warning.message?.includes('"use client"') ||
warning.message?.includes('"use server"'))
) {
return;
}
if (userOnwarn) {
userOnwarn(warning, defaultHandler);
} else {
defaultHandler(warning);
}
};
}Then both branches become a single line: onwarn: createDirectiveOnwarn(config.build?.rollupOptions?.onwarn)
This also makes the onwarn source (rollupOptions vs rolldownOptions) easier to vary per branch.
| /** | ||
| * Get build options config for client builds, version-gated for Vite 8/Rolldown. | ||
| * Vite 7 uses build.rollupOptions, Vite 8+ uses build.rolldownOptions. | ||
| */ | ||
| function getClientBuildOptions(viteVersion: number): { | ||
| rollupOptions?: { | ||
| input?: Record<string, string>; | ||
| output: { manualChunks: typeof clientManualChunks; experimentalMinChunkSize?: number }; | ||
| treeshake: { preset?: "recommended"; moduleSideEffects: "no-external" }; | ||
| }; | ||
| rolldownOptions?: { | ||
| input?: Record<string, string>; | ||
| output: { manualChunks: typeof clientManualChunks }; | ||
| treeshake: { moduleSideEffects: "no-external" }; | ||
| }; | ||
| } { | ||
| if (viteVersion >= 8) { | ||
| // Vite 8+ uses Rolldown - config goes under rolldownOptions | ||
| return { | ||
| rolldownOptions: { | ||
| output: getClientOutputConfig(viteVersion), | ||
| treeshake: getClientTreeshakeConfig(viteVersion), | ||
| }, | ||
| }; | ||
| } | ||
| // Vite 7 uses Rollup - config goes under rollupOptions | ||
| return { | ||
| rollupOptions: { | ||
| output: getClientOutputConfig(viteVersion), | ||
| treeshake: getClientTreeshakeConfig(viteVersion), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Get build options config for client builds with custom input, version-gated for Vite 8/Rolldown. | ||
| * Vite 7 uses build.rollupOptions, Vite 8+ uses build.rolldownOptions. | ||
| */ | ||
| function getClientBuildOptionsWithInput( | ||
| viteVersion: number, | ||
| input: Record<string, string>, | ||
| ): { | ||
| rollupOptions?: { | ||
| input: Record<string, string>; | ||
| output: { manualChunks: typeof clientManualChunks; experimentalMinChunkSize?: number }; | ||
| treeshake: { preset?: "recommended"; moduleSideEffects: "no-external" }; | ||
| }; | ||
| rolldownOptions?: { | ||
| input: Record<string, string>; | ||
| output: { manualChunks: typeof clientManualChunks }; | ||
| treeshake: { moduleSideEffects: "no-external" }; | ||
| }; | ||
| } { | ||
| if (viteVersion >= 8) { | ||
| // Vite 8+ uses Rolldown - config goes under rolldownOptions | ||
| return { | ||
| rolldownOptions: { | ||
| input, | ||
| output: getClientOutputConfig(viteVersion), | ||
| treeshake: getClientTreeshakeConfig(viteVersion), | ||
| }, | ||
| }; | ||
| } | ||
| // Vite 7 uses Rollup - config goes under rollupOptions | ||
| return { | ||
| rollupOptions: { | ||
| input, | ||
| output: getClientOutputConfig(viteVersion), | ||
| treeshake: getClientTreeshakeConfig(viteVersion), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
getClientBuildOptions and getClientBuildOptionsWithInput are near-identical — they only differ in whether input is included. Consider collapsing them into a single function with an optional parameter:
function getClientBuildOptions(
viteVersion: number,
input?: Record<string, string>,
) { ... }This halves the type signature and branching duplication, and eliminates a whole exported function.
| expect(result.build.rollupOptions?.treeshake).toBeUndefined(); | ||
|
|
||
| // Client environment should have treeshake | ||
| expect(result.environments.client.build.rollupOptions.treeshake).toEqual({ |
There was a problem hiding this comment.
This test asserts result.environments.client.build.rollupOptions.treeshake — which is the Vite 7 shape. With the new version-gating in the production code, running on Vite 8 would put the config under rolldownOptions instead, and this assertion would fail.
Since the multi-env paths now use getClientBuildOptionsWithInput() (which returns rolldownOptions for Vite 8), this test only validates the Vite 7 path. Consider adding a comment noting this limitation, or (better) adding a mock-based test that exercises the Vite 8 multi-env path.
| // Vite 8+ uses Rolldown which doesn't support `preset` option | ||
| // moduleSideEffects is still supported in Rolldown | ||
| return { | ||
| moduleSideEffects: "no-external" as const, |
There was a problem hiding this comment.
The comment says "moduleSideEffects is still supported in Rolldown" — has this been verified? The previous bot reviews raised the same concern. If Rolldown ignores rollupOptions.treeshake entirely (requiring rolldownOptions.treeshake instead), then getClientBuildOptions() correctly places it under rolldownOptions for Vite 8 — good. But the standalone (non-multi-env) path in the config hook at line ~1328 also places it correctly.
The remaining question is whether Rolldown actually supports the moduleSideEffects: "no-external" value at all. If it doesn't, the barrel-library DCE optimization is silently lost. A quick test with a Vite 8 build would confirm — consider documenting the verification result in a comment here.
|
Review submitted successfully. Here's a summary of what I flagged on PR #548: Requested Changes (6 inline comments)1. Bug: 2. Multi-env Vite 8 path is untested — The integration test at line 436 asserts 3. Duplicated 4. Broad 5. Near-identical 6. Unverified |
2 participants
Fixes #540