fix: support set NODE_ENV in scripts when custom mode option

[Dunqing]

Description

Support set NODE_ENV in scripts when custom mode option, e.g:

NODE_ENV=production vite build --mode custom && vite preview

Additional context

---

close: #8192

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bluwy]

Should process.env.NODE_ENV be the first? Otherwise if NODE_ENV is define in both CLI args and .env, the .env would take priority. I'm not sure if this causes any side effects, but I also don't remember why we have VITE_USER_NODE_ENV in the first place

[Dunqing]

I started by putting process.env.NODE_ENV in the first place, will causes ci fail https://github.com/vitejs/vite/runs/6491327150?check_suite_focus=true

[Dunqing]

I don't know why, I guess this test-serve would set the default NODE_ENV.

[sapphi-red]

vitest sets NODE_ENV to mode || 'test' when NODE_ENV is not defined.
https://github.com/vitest-dev/vitest/blob/4701e0b9b07728fc64189b655c53708346ac9293/packages/vitest/src/node/cli-api.ts#L18
Maybe this is affecting?

[swandir]

Shouldn't NODE_ENV be just set here

vite/packages/vite/src/node/config.ts

Lines 1011 to 1017 in 79a8c85

	} else if (
	key === 'NODE_ENV' &&
	process.env.VITE_USER_NODE_ENV === undefined
	) {
	// NODE_ENV override in .env file
	process.env.VITE_USER_NODE_ENV = value
	}

to process.env.VITE_USER_NODE_ENV ?? process.env.NODE_ENV ?? value instead in order to both keep respecting externally set VITE_USER_NODE_ENV and allow externally set NODE_ENV as well?

[Dunqing]

And @Dunqing, maybe good to justify why you choose the current approach instead of the one proposed by @swandir if the result is the same?

My intention is to make as few changes as possible to support this implementation, and if possible I can create a new pr separately to optimize this logic

[swandir]

The effect is actually the same

If I'm not mistaken, there's a slight difference.

Currently, when mode is used, and both NODE_ENV and VITE_USER_NODE_ENV are set as system env vars (not in .env file), VITE_USER_NODE_ENV takes precedence.

The change proposed in this PR seems to flip the precedence

- const isProduction = (process.env.VITE_USER_NODE_ENV || mode) === 'production'
+ const isProduction =
+  (process.env.NODE_ENV || process.env.VITE_USER_NODE_ENV || mode) ===
+  'production'

It potentially may affect someone using VITE_USER_NODE_ENV currently.

The change may be acceptable given that the case is pretty obscure, but I feel like it should be taken into consideration.

And once externally set NODE_ENV is respected, there doesn't seem to be a reason for VITE_USER_NODE_ENV to exist.

[Dunqing]

The effect is actually the same

If I'm not mistaken, there's a slight difference.

Currently, when mode is used, and both NODE_ENV and VITE_USER_NODE_ENV are set as system env vars (not in .env file), VITE_USER_NODE_ENV takes precedence.

The change proposed in this PR seems to flip the precedence

- const isProduction = (process.env.VITE_USER_NODE_ENV || mode) === 'production'
+ const isProduction =
+  (process.env.NODE_ENV || process.env.VITE_USER_NODE_ENV || mode) ===
+  'production'

It potentially might affect someone using VITE_USER_NODE_ENV currently.

The change may be acceptable given that the case is pretty obscure, but I feel like it should be taken into consideration.

And once externally set NODE_ENV is respected, there doesn't seem to be a reason for VITE_USER_NODE_ENV to exist.

I think VITE_USER_NODE_ENV is a property provided for internal use, there is no description of VITE_USER_NODE_ENV in the documentation.

[sapphi-red]

Shouldn't build always set it to production unless overruled by external NODE_ENV? If NODE_ENV isn't set, and it is build just set it to production otherwise use mode | <VITEST> NODE_ENV = NODE_ENV ?? (isBuild ? 'production' : mode )

Or am I missing something?

Sorry I forgot to reply.
This docs implies that the default NODE_ENV is not production even if it is build. If the default NODE_ENV were production, NODE_ENV does not need to be set with .env.
https://vitejs.dev/guide/env-and-mode.html#modes
But we might need to consider which is better.

[patak-dev]

And @Dunqing, maybe good to justify why you choose the current approach instead of the one proposed by @swandir if the result is the same?

My intention is to make as few changes as possible to support this implementation, and if possible I can create a new pr separately to optimize this logic

Sounds good, I think we can merge this one then until we do a more holistic refactoring 👍🏼
Thanks for taking the time to address the questions 🙏🏼