fix: don't replace NODE_ENV in vite:client-inject

[patak-dev]

Description

See vitest-dev/vitest#202 (comment)

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[patak-dev]

Checking CI, @sheremet-va I think you are right about this branch being superfluous

[sheremet-va]

Checking CI, @sheremet-va I think you are right about this branch being superfluous

Well, I see CI failing.. 👀

[patak-dev]

@bluwy I saw you did a PR before touching NODE_ENV replacement. Do you know why this branch is needed? Looks to me (as suggested by @sheremet-va) that the replacement done in the DefinePlugin should already take care of process.env.NODE_ENV. Why is this also being replaced here?

[vursen]

I ran into this a couple of days ago when I wanted to configure some env variables for a service worker. It turned out that DefinePlugin only takes place for the production build and the server build (when SSR is enabled).

vite/packages/vite/src/node/plugins/define.ts

Lines 84 to 89 in 3f27d58

	const ssr = options?.ssr === true
	if (!ssr && !isBuild) {
	// for dev we inject actual global defines in the vite client to
	// avoid the transform cost.
	return
	}

As for the development build, it seems to be vite/env.ts that is supposed to define env variables. And it does it at runtime.

For all that, I expected that importing vite/env.ts would define process.env.NODE_ENV. However, the reality was that it gets replaced by this workaround.

So I would be interested in listening to why it was decided to keep this workaround instead of defining all the variables in vite/env.ts at runtime (in dev mode).

[bluwy]

Yeah this look superfluous. The define plugin currently doesn't handle this case though since it bails out and relied on a global globalThis.process.env.NODE_ENV = "env" call for performance.

vite/packages/vite/src/node/plugins/define.ts

Lines 85 to 88 in 56ed808

	if (!ssr && !isBuild) {
	// for dev we inject actual global defines in the vite client to
	// avoid the transform cost.
	return

But it looks like currently we don't do a globalThis.process.env.NODE_ENV = "env" call anywhere, which causes the CI to fail. It should be fixable though if we add process.env.NODE_ENV to the __DEFINES__:

vite/packages/vite/src/node/plugins/clientInjections.ts

Line 46 in 3f27d58

.replace(`__DEFINES__`, serializeDefine(config.define || {}))

The current code now looks like a quick way to get it working before, which causes the Vitest issue since it doesn't handle preventAssignment cases.

[sheremet-va]

So instead of having processNodeEnv in https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/define.ts#L10
we should move it to the the place where Vite resolves config?

[patak-dev]

I'm thinking of directly adding it in client injection fa7e458

[patak-dev]

@antfu @bluwy Im thinking this may be a bigger change than what looked like at the beggining. We are now going to have a process global in dev. So some errors with user code using process will be different (and inconsistent between dev and build). I think this is the reason for doing the replacement out of defines. Maybe we need to avoid replacing it always, or replace with a guarded value instead of config.mode (like we do in the define plugin). I'll check later today, for sure this shouldnt be in a patch

[antfu]

Or I guess we could bypass the replace only for ssr

[bluwy]

Ah I think you're right @patak-dev. I just found 8ad7ecd which prevents this from happening. Anthony's idea would be better.