refactor: introduce mergeWithDefaults and organize how default values for config options are set

[sapphi-red]

Description

Tentative PR to discuss how we should expose the values.

[sapphi-red]

Probably needs a helper function that merges recursively but replaces arrays, slightly different from mergeConfig.

[patak-dev]

That would be a fantastic refactoring 💯

[sapphi-red]

Added mergeWithDefaults function 👍

[sapphi-red]

There's many inferred options. Is it fine to simply omit it?

[bluwy]

I was initially thinking that we'd only export the defaults that're special, like hardcoded array of values. That way we solve the problem of users wanting to extend it rather than completely replacing it.

I'm not sure yet about exposing all the defaults in the object. It seems to be a lot to keep track of, and like you mention here, there's some that's inferred.

[patak-dev]

I'm fine with omitting the inferred ones, maybe adding a comment about what how that inference works. My vote goes for the current implementation in the PR. I think once the "merge" helper is in, it is going to be easier to work with.

[bluwy]

What kind of merge helper are you referring to? What I guess I'm concerned about with exposing tons of options this way is that people could unknowingly used it like vite.build(configDefaults) or use it to merge entire configs which could be wasteful or lead to incorrect merges (like arrays). Hence I was thinking to be a bit conservative here 🤔

[patak-dev]

I was thinking on the mergeWithDefaults that Sapphi implemented. We could prevent vite.build(configDefaults) by checking the instance. But you're right that they may do something like: vite.build({ ...configDefaults, foo: 'bar' }).

I don't think that current users would ever try to do something like this, but depending how we document configDefaults, I see your point.

It feels a bit cumbersome to use if we expose a ton of variables like CONFIG_DEFAULTS_RESOLVE_CONDITIONS. Imagine having to import 3 or 4 of these.

Maybe we could make all the properties in the public configDefaults non-enumerable so they can not iterate over the keys and the user needs to access them one by one. We should get the same properties as the individual variables but with better typing and easier to import. We would still have a internalConfigDefaults that we can iterate to be used in mergeWithDefaults

[sapphi-red]

I'm fine with trimming down the values for the exposed object. I think the object form makes it easier for users to find out the value for each options. But given that we already need to expose mainFields and conditions separately, maybe that advantage is gone.

[bluwy]

mergeWithDefaults makes sense to me, I think we needed that elsewhere regardless of this PR too.

I guess given:

1. We couldn't expose all defaults as some are inferred.
2. Some defaults are based on the client / server consumer (mainFields and conditions), which Sapphi has now refactored as DEFAULT_CLIENT_MAIN_FIELDS etc (refactor: introduce mergeWithDefaults and organize how default values for config options are set #18550 (comment))
3. And personally I think I'm missing the usecase where users would want to know the defaults via configDefaults. Isn't the problem we want to solve here that if someone wants to add a new condition on resolve.conditions but want to extend from Vite defaults, they can use this new export? What are the usecase for e.g. configDefaults.base?

I think I'm leaning more on simply exporting the constants like what Sapphi has now with DEFAULT_CLIENT_MAIN_FIELDS. We only export the ones that solves the usecase I mentioned in no3.

[patak-dev]

Maybe it is a good idea to merge only the refactoring that makes things cleaner internally for now, and then we can discuss how to expose the defaults in another PR. Maybe we don't need to do this for Vite 6, people can still copy and past the defaults from the docs (it isn't the same though, that will fix the values, instead of adding an extra to the defaults that is what we have now but it may be ok)

[sapphi-red]

I pushed a commit that removes the new exports so that we can separate the discussion about exposing the default values. 👍

[sapphi-red]

It might be confusing that configDefaults.resolve.conditions contains both node and browser.

[patak-dev]

Maybe we need serverConfigDefaults.resolve.conditions? This is a hard one. I think as you say having both values would be confusing.

Or maybe we should omit this one. And treat it as inferred. We could expose some defaults in a special way?
clientDefaults.resolve.conditions, serverDefaults.resolve.conditions?

[sapphi-red]

I removed mainFields and conditions from configDefaults and exposed the values separately. I think clientDefaults / serverDefaults might be confusing as it is also used for ssr.target === 'webworker'. If we remove all isSsrTargetWebworkerEnvironment in future, I think we can have clientDefaults / serverDefaults that contains all values that is applied conditionally by consumer.

[sapphi-red]

I noticed that this only exposes the value for ESM entry point and does not expose the value for CJS entry point. Are we fine with that? To make that work, configDefaults and the values used in it needs to be moved to constants.ts and makes a bit difficult to read the code.

[bluwy]

Yeah I think we'd have to expose it in CJS for consistency unfortunately, otherwise they could get a confusing error of it not working.

[patak-dev]

this is awesome

[sapphi-red]

I fixed unocss by a5e77f0 and 3313e83 and also fixed vite-setup-catalogue by fd4ccb9.

The other fails that was not failing for latest scheduled are:

• marko: failing because we now inline SVGs in dev (feat(asset): inline svg in dev if within limit #18581)
• qwik: will check later
• storybook: will check later
• vite-environment-examples: type incompatibility caused by feat: use a single transport for fetchModule and HMR support #18362

[sapphi-red]

qwik should be fixed by 6a52f2f
It seems storybook is also failing in main (https://discord.com/channels/804011606160703521/1304311018905800767/1304340637319041085)

[yannbf]

qwik should be fixed by 6a52f2f It seems storybook is also failing in main (discord.com/channels/804011606160703521/1304311018905800767/1304340637319041085)

You already know but just for context for others:
Storybook started to fail because of an older commit, unrelated to this PR: 2ebe4b4#diff-89bae1df62862bb7f4a03d82a1e9cbf4ac6d0c042f21fbbacb0a2238bd050042R430

It only started to fail now because we updated our tests to also run our build command.

[pkg-pr-new]

Open in Stackblitz

pnpm add https://pkg.pr.new/vite@18550

commit: e4364e5

[sapphi-red]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on a2705e9: Open

suite	result	latest scheduled
astro	❌ failure	❌ failure
histoire	❌ failure	❌ failure
marko	❌ failure	❌ failure
redwoodjs	❌ failure	❌ failure
remix	❌ failure	❌ failure
storybook	❌ failure	❌ failure
sveltekit	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-environment-examples	❌ failure	✅ success
vite-plugin-react	❌ failure	❌ failure
vite-plugin-svelte	❌ failure	❌ failure
vite-plugin-vue	✅ success	❌ failure
vitest	❌ failure	❌ failure

✅ analogjs, ladle, laravel, nuxt, previewjs, quasar, qwik, rakkas, unocss, vite-plugin-pwa, vite-plugin-react-swc, vite-setup-catalogue, vitepress, vuepress

[sapphi-red]

It should be fine now.

[patak-dev]

Let's merge this one @bluwy? note: there is nothing new exposed now

[bluwy]

Since arrays, objects and functions are already handled above, what cases are covered here with the structuredClone? From a quick test, structuredClone seems to have a big perf impact when used on basic primitives.

[sapphi-red]

It was mainly for RegExp. I put the structuredClone in a condition 👍 (38162a0, e4364e5).

[bluwy]

I don't think it's likely for someone to mutate a RegExp, but I don't mind having the code for it either ways.

[bluwy]

Looks great! Thanks for updating based on the suggestions.