Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/vinext/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* needed for most Next.js apps.
*/

import vinext, { clientOutputConfig, clientTreeshakeConfig } from "./index.js";
import vinext, { getClientOutputConfig, getClientTreeshakeConfig } from "./index.js";
import { printBuildReport } from "./build/report.js";
import path from "node:path";
import fs from "node:fs";
Expand Down Expand Up @@ -350,6 +350,7 @@ async function buildApp() {
console.log(`\n vinext build (Vite ${getViteVersion()})\n`);

const isApp = hasAppDir();
const viteMajorVersion = parseInt(getViteVersion().split(".")[0], 10);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
const viteMajorVersion = parseInt(getViteVersion().split(".")[0], 10);
const viteMajorVersion = getViteMajorVersion();

(This would also require adding getViteMajorVersion to the import from ./index.js.)

// In verbose mode, skip the custom logger so raw Vite/Rollup output is shown.
const logger = parsed.verbose
? vite.createLogger("info", { allowClearScreen: false })
Expand Down Expand Up @@ -387,8 +388,8 @@ async function buildApp() {
ssrManifest: true,
rollupOptions: {
input: "virtual:vinext-client-entry",
output: clientOutputConfig,
treeshake: clientTreeshakeConfig,
output: getClientOutputConfig(viteMajorVersion),
treeshake: getClientTreeshakeConfig(viteMajorVersion),
},
},
},
Expand Down
54 changes: 47 additions & 7 deletions packages/vinext/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,37 @@ const clientTreeshakeConfig = {
moduleSideEffects: "no-external" as const,
};

/**
* Get Rollup-compatible output config for client builds.
* Returns config without Vite 8/Rolldown-incompatible options.
*/
function getClientOutputConfig(viteVersion: number) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 } {

if (viteVersion >= 8) {
// Vite 8+ uses Rolldown which doesn't support experimentalMinChunkSize
return {
manualChunks: clientManualChunks,
};
}
// Vite 7 uses Rollup with experimentalMinChunkSize support
return clientOutputConfig;
}

/**
* Get Rollup-compatible treeshake config for client builds.
* Returns config without Vite 8/Rolldown-incompatible options.
*/
function getClientTreeshakeConfig(viteVersion: number) {
if (viteVersion >= 8) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Vite 8+ uses Rolldown which doesn't support `preset` option
// moduleSideEffects is still supported in Rolldown
return {
moduleSideEffects: "no-external" as const,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
}
// Vite 7 uses Rollup with preset support
return clientTreeshakeConfig;
}

type BuildManifestChunk = {
file: string;
isEntry?: boolean;
Expand Down Expand Up @@ -1232,15 +1263,17 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {
// avoid leaking into RSC/SSR environments where
// moduleSideEffects: 'no-external' could drop server packages
// that rely on module-level side effects.
...(!isSSR && !isMultiEnv ? { treeshake: clientTreeshakeConfig } : {}),
...(!isSSR && !isMultiEnv
? { treeshake: getClientTreeshakeConfig(viteMajorVersion) }
: {}),
// Code-split client bundles: separate framework (React/ReactDOM),
// vinext runtime (shims), and vendor packages into their own
// chunks so pages only load the JS they need.
// Only apply globally for standalone client builds (CLI Pages
// Router). For multi-environment builds (App Router, Cloudflare),
// manualChunks is set per-environment on the client env below
// to avoid leaking into RSC/SSR environments.
...(!isSSR && !isMultiEnv ? { output: clientOutputConfig } : {}),
...(!isSSR && !isMultiEnv ? { output: getClientOutputConfig(viteMajorVersion) } : {}),
},
},
// Let OPTIONS requests pass through Vite's CORS middleware to our
Expand Down Expand Up @@ -1444,8 +1477,8 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {
...(hasCloudflarePlugin ? { manifest: true } : {}),
rollupOptions: {
input: { index: VIRTUAL_APP_BROWSER_ENTRY },
output: clientOutputConfig,
treeshake: clientTreeshakeConfig,
output: getClientOutputConfig(viteMajorVersion),
treeshake: getClientTreeshakeConfig(viteMajorVersion),
},
},
},
Expand All @@ -1463,8 +1496,8 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {
ssrManifest: true,
rollupOptions: {
input: { index: VIRTUAL_CLIENT_ENTRY },
output: clientOutputConfig,
treeshake: clientTreeshakeConfig,
output: getClientOutputConfig(viteMajorVersion),
treeshake: getClientTreeshakeConfig(viteMajorVersion),
},
},
},
Expand Down Expand Up @@ -3752,7 +3785,14 @@ export type {
export type { NextConfig } from "./config/next-config.js";

// Exported for CLI and testing
export { clientManualChunks, clientOutputConfig, clientTreeshakeConfig, computeLazyChunks };
export {
clientManualChunks,
clientOutputConfig,
clientTreeshakeConfig,
computeLazyChunks,
getClientOutputConfig,
getClientTreeshakeConfig,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = { ... };

};
export { augmentSsrManifestFromBundle as _augmentSsrManifestFromBundle };
export { resolvePostcssStringPlugins as _resolvePostcssStringPlugins };
export { _postcssCache };
Expand Down
Loading