Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
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
11 changes: 8 additions & 3 deletions packages/vinext/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@
* needed for most Next.js apps.
*/

import vinext, { clientOutputConfig, clientTreeshakeConfig } from "./index.js";
import vinext, {
getClientOutputConfig,
getClientTreeshakeConfig,
getViteMajorVersion,
} 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 +354,7 @@ async function buildApp() {
console.log(`\n vinext build (Vite ${getViteVersion()})\n`);

const isApp = hasAppDir();
const viteMajorVersion = getViteMajorVersion();
// 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 +392,8 @@ async function buildApp() {
ssrManifest: true,
rollupOptions: {
input: "virtual:vinext-client-entry",
output: clientOutputConfig,
treeshake: clientTreeshakeConfig,
output: getClientOutputConfig(viteMajorVersion),
treeshake: getClientTreeshakeConfig(viteMajorVersion),
},
},
},
Expand Down
60 changes: 52 additions & 8 deletions packages/vinext/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ function extractStaticValue(node: any): unknown {
* by a project that has Vite 8 — so we resolve from cwd, not from
* the plugin's own location.
*/
function getViteMajorVersion(): number {
export function getViteMajorVersion(): number {
try {
const require = createRequire(path.join(process.cwd(), "package.json"));
const vitePkg = require("vite/package.json");
Expand Down Expand Up @@ -525,6 +525,41 @@ 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,
): typeof clientOutputConfig | { manualChunks: typeof clientManualChunks } {
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.

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:

Suggested change
): 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.

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,
): typeof clientTreeshakeConfig | { moduleSideEffects: "no-external" } {
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 +1267,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 +1481,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 +1500,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 +3789,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
47 changes: 47 additions & 0 deletions tests/build-optimization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@
import { describe, it, expect, beforeEach, afterEach } from "vitest";
import {
clientManualChunks,
clientOutputConfig,
clientTreeshakeConfig,
computeLazyChunks,
_augmentSsrManifestFromBundle,
_stripServerExports,
_asyncHooksStubPlugin,
getClientOutputConfig,
getClientTreeshakeConfig,
} from "../packages/vinext/src/index.js";

// The vinext config hook mutates process.env.NODE_ENV as a side effect (matching
Expand Down Expand Up @@ -47,6 +50,50 @@ describe("clientTreeshakeConfig", () => {
});
});

// ─── getClientOutputConfig / getClientTreeshakeConfig ────────────────────────

describe("getClientOutputConfig", () => {
it("returns full config with experimentalMinChunkSize for Vite 7", () => {
const result = getClientOutputConfig(7);
expect(result).toEqual(clientOutputConfig);
expect((result as any).experimentalMinChunkSize).toBe(10_000);
expect(result.manualChunks).toBe(clientManualChunks);
});

it("returns config without experimentalMinChunkSize for Vite 8", () => {
const result = getClientOutputConfig(8);
expect(result).toEqual({ manualChunks: clientManualChunks });
expect(result).not.toHaveProperty("experimentalMinChunkSize");
});

it("returns config without experimentalMinChunkSize for Vite 9", () => {
const result = getClientOutputConfig(9);
expect(result).toEqual({ manualChunks: clientManualChunks });
expect(result).not.toHaveProperty("experimentalMinChunkSize");
});
});

describe("getClientTreeshakeConfig", () => {
it("returns full config with preset for Vite 7", () => {
const result = getClientTreeshakeConfig(7);
expect(result).toEqual(clientTreeshakeConfig);
expect((result as any).preset).toBe("recommended");
expect(result.moduleSideEffects).toBe("no-external");
});

it("returns config without preset for Vite 8", () => {
const result = getClientTreeshakeConfig(8);
expect(result).toEqual({ moduleSideEffects: "no-external" });
expect(result).not.toHaveProperty("preset");
});

it("returns config without preset for Vite 9", () => {
const result = getClientTreeshakeConfig(9);
expect(result).toEqual({ moduleSideEffects: "no-external" });
expect(result).not.toHaveProperty("preset");
});
});

// ─── clientManualChunks ───────────────────────────────────────────────────────

describe("clientManualChunks", () => {
Expand Down
Loading