mergeConfig doesn't merge null or undefined values

[cmd-johnson]

Describe the bug

When using mergeConfig, it is impossible to merge null or undefined values into the base config.

I've encountered this while using Remix ≥ 2.14.0, where vite.mergeConfig is used to merge a value for server.watch into the base config. This doesn't work for null or undefined values, which are the only two values Remix tries to merge.
(See here for the mergeConfig call and here for the data that's passed into it).
This results in the server.watch value never actually being set to anything in the final config, which in turn leads to a whole bunch of files being watched by vite that don't actually need watching during a build.

I'm not sure what the rationale was for excluding null and undefined values when merging values, but I do see some use-cases for allowing them to be merged, like how remix tries (and fails) to do it.

Without additional context, I would expect any values in the overrides record to overwrite the default value, no matter what its value is. If I don't want to override a value, I shouldn't include its key in the overrides in the first place, so even undefined shouldn't receive special treatment as far as I'm concerned.

I've not submitted a PR because I'm not sure what other unforeseen side-effects the removal of that check would have.

I'd also be fine with keeping the check as-is, but then at the very least this quirk should be included in the documentation.

Reproduction

https://stackblitz.com/edit/vitejs-vite-66qrvrft?file=index.js

Steps to reproduce

No response

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    vite: ^6.0.3 => 6.0.3

Used Package Manager

npm

Logs

{
  baseConfig: { server: { preTransformRequests: false, hmr: false } },
  overrideConfig: { server: { watch: null, otherKey: undefined } },
  mergedConfig: { server: { preTransformRequests: false, hmr: false } }
}

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[sapphi-red]

I guess the reason why undefined was excluded is to make it easy to conditionally set some values. For example,

// this won't work if `undefined` is not excluded
mergeConfig(originalConfig, {
  envDir: client ? './something' : undefined
})

// it has to be like
mergeConfig(originalConfig, {
  ...(client ? { envDir: './something' } : {})
})

For null, I guess it was just because of the "null should be the same behavior with undefined" philosophy.

I think it's more intuitive if we change this line to value === undefined.

vite/packages/vite/src/node/utils.ts

Line 1171 in 3400a5e

if (value == null) {

But this condition has been there from the beginning so may cause some breakage.

I wonder if we should receive false instead of null for options that now receives null.

[btea]

I think if we want to adjust for this situation, maybe we can add a new parameter skipNil with a default value of true.

vite/packages/vite/src/node/utils.ts

Lines 1225 to 1231 in 3400a5e

	export function mergeConfig<
	D extends Record<string, any>,
	O extends Record<string, any>,
	>(
	defaults: D extends Function ? never : D,
	overrides: O extends Function ? never : O,
	isRoot = true,

[sapphi-red]

I think if we want to adjust for this situation, maybe we can add a new parameter skipNil with a default value of true.

That would make the behavior inconsistent between frameworks and I think that is confusing. The users of mergeConfig will need to set skipNil to true and not forget to do it.

[btea]

The users of mergeConfig will need to set skipNil to true and not forget to do it.

What I want is to give the parameter a default value of true, so that it will be consistent with the original default behavior when no value is passed.