fix: set correct NODE_ENV for build by hyoban · Pull Request #247 · cloudflare/vinext

hyoban · 2026-03-04T01:56:14Z

Fixes #246

southpolesteve · 2026-03-04T17:20:33Z

/bigbonk review

ask-bonk

Thanks for the fix — setting NODE_ENV correctly during build is important. The approach is sound overall: align process.env.NODE_ENV with Next.js semantics and inject it as a define for static replacement. A few things to tighten up below.

ask-bonk · 2026-03-04T17:24:08Z

+        // Align NODE_ENV with Next.js semantics: build -> production, serve -> development.
+        const resolvedNodeEnv = mode === "test"


The ternary chain works but is a bit hard to parse at a glance. Consider using a local helper or an if/else if/else block for clarity:

Suggested change

// Align NODE_ENV with Next.js semantics: build -> production, serve -> development.

const resolvedNodeEnv = mode === "test"

// Align NODE_ENV with Next.js semantics: build -> production, serve -> development.

let resolvedNodeEnv: string;

if (mode === "test") {

resolvedNodeEnv = "test";

} else if (env?.command === "build") {

resolvedNodeEnv = "production";

} else {

resolvedNodeEnv = "development";

}

Nit — feel free to ignore.

ask-bonk · 2026-03-04T17:24:08Z

+        if (
+          !config.define ||
+          typeof config.define !== "object" ||
+          !("process.env.NODE_ENV" in config.define)


The guard typeof config.define !== "object" is redundant — Vite's UserConfig["define"] is typed as Record<string, any> | undefined, so if it's truthy it's always an object. But more importantly, this check has a subtle issue: if the user passes config.define as an empty object {}, the in check returns false and vinext still injects its value — which is the correct behavior. That's fine.

However, the typeof config.define !== "object" branch would also catch null, which while unlikely, would cause "process.env.NODE_ENV" in null to throw. So the typeof guard does serve a defensive purpose. It's fine as-is — just noting for clarity that the key protection here is really the !config.define check.

ask-bonk · 2026-03-04T17:24:08Z

+      const mockConfig = { root: tmpDir, build: {}, plugins: [] };
+      const result = await (mainPlugin as any).config(
+        mockConfig,
+        { command: "build", mode: "production" },


The test only covers the { command: "build", mode: "production" } case. It would be valuable to also test:

command: "serve" — verify process.env.NODE_ENV is "development" (or not set as "production")

command: "build" without mode — the existing tests in this file call config(mockConfig, { command: "build" }) (no mode). With the new code, mode falls back to "development" via env?.mode ?? "development" on line 1692, yet env?.command is "build" so resolvedNodeEnv becomes "production". This is correct Next.js behavior but isn't explicitly tested.

User already defined process.env.NODE_ENV — verify the guard at line 1747-1751 respects user overrides.

At minimum, case (2) would be good to add since it matches what the other tests in this file actually do (pass { command: "build" } with no mode).

ask-bonk · 2026-03-04T17:24:08Z

+let originalNodeEnv: string | undefined;
+
+beforeEach(() => {
+  originalNodeEnv = process.env.NODE_ENV;
+});
+
+afterEach(() => {
+  if (originalNodeEnv === undefined) {
+    delete process.env.NODE_ENV;
+  } else {
+    process.env.NODE_ENV = originalNodeEnv;
+  }
+});


The beforeEach/afterEach for saving and restoring NODE_ENV is good practice since the production code now mutates process.env.NODE_ENV. However, these hooks are at the file top level (outside any describe block), which means they run for every test in the file — including unrelated tests like clientTreeshakeConfig, clientManualChunks, and computeLazyChunks.

Consider scoping these hooks inside the process.env.NODE_ENV define describe block instead, so the save/restore only runs for tests that actually need it. Or, if you're concerned about the existing tests being affected by the new process.env.NODE_ENV mutation in the config hook, that's a fair reason to keep them global — but it would be worth adding a comment explaining why.

ask-bonk · 2026-03-04T17:24:19Z

Review posted successfully. Here's a summary of my feedback on PR #247:

Overall: The approach is correct — aligning process.env.NODE_ENV with Next.js semantics (build = production, serve = development) and injecting it as a define for static replacement in client bundles. This is safe and won't conflict with Vite's built-in NODE_ENV handling since user/plugin defines take precedence.

Comments posted (4):

Ternary readability (index.ts:1700-1701) — Minor style nit suggesting if/else if/else over nested ternaries for clarity.
Guard condition (index.ts:1747-1750) — Noted that the typeof config.define !== "object" check is mostly redundant given Vite's types, though it does defensively handle the null case. Fine as-is.
Test coverage gaps (build-optimization.test.ts:205) — The test only covers { command: "build", mode: "production" }. Suggested adding cases for: command: "serve" (should be "development"), command: "build" without mode (matches what other tests in the file do), and user-defined process.env.NODE_ENV override.
beforeEach/afterEach scope (build-optimization.test.ts:15-27) — The NODE_ENV save/restore hooks are at the file top level, affecting all tests including unrelated ones. Suggested scoping them inside the relevant describe block or adding a comment explaining why they're global.

github run

…nv guard - Replace nested ternary with if/else for NODE_ENV resolution - Add comment explaining why beforeEach/afterEach is file-scoped - Add test cases: serve (development), build without mode, user override - Skip NODE_ENV key in nextConfig.env loop to prevent silent override - Extract setupTmpProject helper to reduce test boilerplate

southpolesteve

Addressed all review feedback and added an additional fix:

Review feedback addressed:

Ternary readability - Replaced nested ternary with if/else block for the NODE_ENV resolution logic.
beforeEach/afterEach scope - Kept file-scoped (necessary because other test blocks like optimizeDeps and treeshake also call mainPlugin.config(), which now mutates process.env.NODE_ENV). Added a comment explaining why.
Test coverage - Added three new test cases:
- command: "serve" verifies "development"
- command: "build" without explicit mode verifies "production"
- User-defined config.define with process.env.NODE_ENV verifies the guard skips injection
Guard condition - Left as-is; the typeof check defensively handles the null case.

Additional fix (from my own review):

The nextConfig.env loop (which injects env vars from next.config.js) could silently override the NODE_ENV define we just set. If a user had env: { NODE_ENV: "development" } in their next.config, it would overwrite "production" during build. Added a if (key === "NODE_ENV") continue guard, matching Next.js behavior (Next.js also ignores NODE_ENV in the env config key).

Also extracted a setupTmpProject() helper in the test file to reduce boilerplate across the NODE_ENV test cases.

All checks pass locally: typecheck clean, lint 0 warnings, 37/37 build-optimization tests pass, 1041/1041 core tests pass.

pkg-pr-new · 2026-03-04T21:57:11Z

Open in StackBlitz

npm i https://pkg.pr.new/vinext@247

commit: a46d664

nunofgs · 2026-03-21T16:48:45Z

👋 thanks for the fix. Has this been included in a release?

james-elicx · 2026-03-21T17:02:59Z

👋 thanks for the fix. Has this been included in a release?

Hi @nunofgs, it was first released in v0.0.20.

fix: set NODE_ENV

395760f

hyoban mentioned this pull request Mar 4, 2026

chore: add vinext as dev server langgenius/dify#32559

Merged

5 tasks

ask-bonk Bot reviewed Mar 4, 2026

View reviewed changes

southpolesteve reviewed Mar 4, 2026

View reviewed changes

southpolesteve enabled auto-merge (squash) March 4, 2026 21:56

southpolesteve merged commit 5b9d775 into cloudflare:main Mar 4, 2026
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: set correct NODE_ENV for build#247

fix: set correct NODE_ENV for build#247
southpolesteve merged 2 commits intocloudflare:mainfrom
hyoban:3-4-set-NODE_ENV

hyoban commented Mar 4, 2026

Uh oh!

southpolesteve commented Mar 4, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Mar 4, 2026

Uh oh!

ask-bonk Bot Mar 4, 2026

Uh oh!

ask-bonk Bot Mar 4, 2026

Uh oh!

ask-bonk Bot Mar 4, 2026

Uh oh!

ask-bonk Bot commented Mar 4, 2026

Uh oh!

southpolesteve left a comment

Uh oh!

pkg-pr-new Bot commented Mar 4, 2026

Uh oh!

Uh oh!

nunofgs commented Mar 21, 2026

Uh oh!

james-elicx commented Mar 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		// Align NODE_ENV with Next.js semantics: build -> production, serve -> development.
		const resolvedNodeEnv = mode === "test"

-        // Align NODE_ENV with Next.js semantics: build -> production, serve -> development.
-        const resolvedNodeEnv = mode === "test"
+        // Align NODE_ENV with Next.js semantics: build -> production, serve -> development.
+        let resolvedNodeEnv: string;
+        if (mode === "test") {
+          resolvedNodeEnv = "test";
+        } else if (env?.command === "build") {
+          resolvedNodeEnv = "production";
+        } else {
+          resolvedNodeEnv = "development";
+        }

Conversation

hyoban commented Mar 4, 2026

Uh oh!

southpolesteve commented Mar 4, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 4, 2026

Uh oh!

southpolesteve left a comment

Choose a reason for hiding this comment

Uh oh!

pkg-pr-new Bot commented Mar 4, 2026

Uh oh!

Uh oh!

nunofgs commented Mar 21, 2026

Uh oh!

james-elicx commented Mar 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants