fix(build): eliminate Vite 8 treeshake.preset warning#746
fix(build): eliminate Vite 8 treeshake.preset warning#746james-elicx merged 4 commits intocloudflare:mainfrom
Conversation
Add version-gated getClientTreeshakeConfigForVite() function that returns Rollup-compatible config (with preset) for Vite 7 and Rolldown-compatible config (without preset) for Vite 8+. The treeshake.preset option is Rollup-specific and causes warnings in Vite 8 which uses Rolldown. The moduleSideEffects: 'no-external' option is valid in both bundlers and is preserved in all configurations. Changes: - Add getClientTreeshakeConfigForVite(viteMajorVersion) function - Update 4 usage sites to use version-gated function - Mark clientTreeshakeConfig as @deprecated for backward compatibility - Export getClientTreeshakeConfigForVite for testing - Add comprehensive tests for Vite 7/8/9+ compatibility - Update existing tests to expect Vite 8 format Fixes cloudflare#540
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): eliminate Vite 8 treeshake.preset warning
The approach is correct and follows the established pattern (getClientOutputConfigForVite, withBuildBundlerOptions). The implementation and tests are clean. Two issues to address before merging:
1. Missing cleanup of the existing playground workaround (blocking)
examples/app-router-playground/vite.config.ts:90-108 has a plugin called strip-rolldown-incompatible-rollup-options that manually deletes treeshake.preset at configResolved time — this is the exact workaround that this PR is supposed to replace. With getClientTreeshakeConfigForVite in place, the preset is never emitted on Vite 8, so that workaround is now dead code. It should be removed (or at least the treeshake.preset deletion part of it) in this PR to avoid leaving stale workarounds in the codebase.
Note: the same plugin also deletes output.experimentalMinChunkSize, which is handled by the existing getClientOutputConfigForVite — so the entire plugin is likely dead code and can be removed.
2. Vite 8 recommended preset behavior gap
For Vite 7, the recommended preset sets multiple treeshake options beyond just moduleSideEffects — it also sets annotations: true, correctContext: true, and tryCatchDeoptimization: true (among others). For Vite 8+, this PR only carries forward moduleSideEffects: "no-external" and drops all the other settings the preset provides.
This is likely fine since Rolldown's defaults are probably sensible, but it's worth confirming and documenting. A comment noting "Rolldown's defaults already cover what the Rollup 'recommended' preset provides" (if true) would help future readers understand this isn't an accidental regression.
| return { | ||
| moduleSideEffects: "no-external" as const, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The Rollup recommended preset sets more than just moduleSideEffects — it also enables annotations: true, correctContext: true, tryCatchDeoptimization: true, etc. Dropping these for Vite 8+ is probably fine if Rolldown's defaults already cover them, but worth confirming and documenting here. A one-liner like:
| } | |
| // Rolldown (Vite 8+) - no preset support, only specific options. | |
| // Rolldown's built-in defaults already cover what Rollup's 'recommended' | |
| // preset provides (annotations, correctContext, tryCatchDeoptimization). | |
| return { |
would help future readers understand this isn't an accidental loss of optimizations.
tests/build-optimization.test.ts
Outdated
|
|
||
| describe("getClientTreeshakeConfigForVite", () => { | ||
| it("returns preset for Vite 7 (Rollup compatibility)", async () => { | ||
| const { getClientTreeshakeConfigForVite } = await import("../packages/vinext/src/index.js"); |
There was a problem hiding this comment.
Nit: All three test cases do await import("../packages/vinext/src/index.js") to get getClientTreeshakeConfigForVite. Since this is a pure function with no side effects or module-level state dependency, you could import it once at the top of the describe block (or at the file level alongside clientManualChunks, clientTreeshakeConfig, etc. on line 13-18) instead of doing a dynamic import in each test. The existing clientTreeshakeConfig and clientManualChunks tests use static imports.
This is minor — the tests work fine as-is.
|
Review submitted. Here's a summary of the feedback: Requested changes on PR #746:
|
Remove dead workaround plugin from playground - Deleted strip-rolldown-incompatible-rollup-options plugin from examples/app-router-playground/vite.config.ts - This workaround is no longer needed since getClientTreeshakeConfigForVite() never emits treeshake.preset for Vite 8+ Add documentation comment about Rolldown defaults - Enhanced JSDoc for getClientTreeshakeConfigForVite() to explain behavior gap - Documents how Rolldown defaults compare to Rollup's recommended preset - Clarifies that moduleSideEffects: no-external is the key optimization Fix test import style - Changed from dynamic await import() to static imports - Matches existing test patterns in build-optimization.test.ts - Removes async from test cases that don't need it Addresses review comments from PR cloudflare#746
|
/bigbonk review |
There was a problem hiding this comment.
Good change — the version-gating follows the established getClientOutputConfigForVite / withBuildBundlerOptions pattern, the playground workaround is cleaned up, and the JSDoc documenting the Rollup preset options vs Rolldown defaults is thorough. Two minor notes below, neither blocking.
packages/vinext/src/index.ts
Outdated
| * - annotations: true (Rolldown default is also true) | ||
| * - manualPureFunctions: [] (Rolldown default is also []) | ||
| * - propertyReadSideEffects: true (Rolldown equivalent is 'always', the default) | ||
| * - unknownGlobalSideEffects: false (Rolldown default is true, slightly more conservative) |
There was a problem hiding this comment.
Nit: This line documents that Rolldown defaults to unknownGlobalSideEffects: true while the Rollup recommended preset sets it to false. That's a real behavioral difference — Rolldown will be more conservative about eliminating code that touches unknown globals.
For a client bundle this is almost certainly fine (maybe marginally larger output), but it's worth calling out explicitly that this is a known divergence, not just a "Rolldown handles it differently" situation. Consider adding (slightly less aggressive DCE on unknown globals — acceptable for client bundles) or similar so future readers don't wonder whether we should be setting unknownGlobalSideEffects: false explicitly on Vite 8+.
packages/vinext/src/index.ts
Outdated
| */ | ||
| function getClientTreeshakeConfigForVite(viteMajorVersion: number) { | ||
| if (viteMajorVersion >= 8) { | ||
| // Rolldown (Vite 8+) - no preset support, only specific options |
There was a problem hiding this comment.
The bonk review suggested expanding this inline comment to note that Rolldown's defaults already cover the other recommended preset options. The JSDoc above does cover it, but the inline comment is what you see when reading the code without scrolling up. Mildly helpful to add the one-liner:
| // Rolldown (Vite 8+) - no preset support, only specific options | |
| // Rolldown (Vite 8+) - no preset support, only specific options. | |
| // Rolldown's built-in defaults already cover what Rollup's 'recommended' | |
| // preset provides (annotations, correctContext, tryCatchDeoptimization). |
|
Review posted. Here's a summary of my feedback: Overall: The PR looks good. It follows the established version-gating pattern ( Two minor (non-blocking) comments:
|
Address PR cloudflare#746 review feedback from @ask-bonk: - JSDoc: Explicitly call out unknownGlobalSideEffects as a known acceptable divergence (Rolldown defaults to true vs Rollup recommended preset's false). Makes it clear this is intentional, not an oversight. - Inline comment: Note that Rolldown's built-in defaults already cover what Rollup's 'recommended' preset provides (annotations, correctContext, tryCatchDeoptimization). Provides immediate context at the code site. Both changes are documentation-only; no functional impact.
|
/bigbonk review |
There was a problem hiding this comment.
Good change. The version-gating pattern matches getClientOutputConfigForVite / withBuildBundlerOptions, the playground workaround removal is correct, the JSDoc is thorough, and the tests use static imports matching the rest of the file. Two minor notes inline.
| // that rely on module-level side effects. | ||
| ...(!isSSR && !isMultiEnv ? { treeshake: clientTreeshakeConfig } : {}), | ||
| ...(!isSSR && !isMultiEnv | ||
| ? { treeshake: getClientTreeshakeConfigForVite(viteMajorVersion) } |
There was a problem hiding this comment.
Not part of this diff, but the comment at line 1483 still references the deprecated clientTreeshakeConfig. Since you're updating all the call sites, worth updating that comment to point to getClientTreeshakeConfigForVite as well.
tests/build-optimization.test.ts
Outdated
| expect(config).toEqual({ | ||
| moduleSideEffects: "no-external", | ||
| }); | ||
| expect(config).not.toHaveProperty("preset"); |
There was a problem hiding this comment.
The not.toHaveProperty("preset") assertions (here and line 1659) are redundant — toEqual does an exact structural match, so if preset were present the toEqual would already fail. Not blocking, but it's noise that could be dropped.
|
Review submitted. Here's a summary of my feedback on PR #746: Overall: The PR looks good. It correctly follows the established version-gating patterns ( Two minor (non-blocking) notes:
|
- Update comment to reference getClientTreeshakeConfigForVite instead of deprecated clientTreeshakeConfig
- Remove redundant not.toHaveProperty('preset') assertions from tests (toEqual already does exact matching)
Addresses review feedback from @ask-bonk[bot] on PR cloudflare#745
|
/bigbonk review |
There was a problem hiding this comment.
Clean change. The version-gating follows the established getClientOutputConfigForVite / withBuildBundlerOptions pattern, the playground workaround plugin is properly removed, the JSDoc thoroughly documents the Rollup preset vs Rolldown defaults divergence, and the tests use static imports matching the rest of the file. All prior review feedback has been addressed.
LGTM.
|
Approved the PR. Here's my assessment: The PR is clean and ready to merge. It:
|
2 participants
Summary
Fixes #540 - Vite 8 builds produce warnings about the deprecated "treeshake.preset" option.
Problem
The "treeshake.preset" option is Rollup-specific and not supported by Rolldown (Vite 8's bundler). This causes build warnings:
Solution
Added version-gated "getClientTreeshakeConfigForVite(viteMajorVersion)" function that returns:
{ preset: "recommended", moduleSideEffects: "no-external" }(Rollup-compatible){ moduleSideEffects: "no-external" }(Rolldown-compatible, no preset)The "moduleSideEffects: "no-external"" option is valid in both bundlers and provides the key optimization for barrel-exporting libraries.
Changes
Testing
Backward Compatibility