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

sapphi-red · 2024-11-01T09:03:12Z

Description

Tentative PR to discuss how we should expose the values.

sapphi-red · 2024-11-01T09:04:04Z

packages/vite/src/node/build.ts

  const resolved: ResolvedBuildEnvironmentOptions = {
-    target: 'modules',
-    cssTarget: false,
+    ...configDefaults.build,
    ...userBuildEnvironmentOptions,
    commonjsOptions: {
-      include: [/node_modules/],
-      extensions: ['.js', '.cjs'],
+      ...configDefaults.build.commonjsOptions,
      ...userBuildEnvironmentOptions.commonjsOptions,
    },
    dynamicImportVarsOptions: {
-      warnOnError: true,
-      exclude: [/node_modules/],
+      ...configDefaults.build.dynamicImportVarsOptions,
      ...userBuildEnvironmentOptions.dynamicImportVarsOptions,
    },


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

That would be a fantastic refactoring 💯

Added mergeWithDefaults function 👍

sapphi-red · 2024-11-01T09:04:45Z

packages/vite/src/node/config.ts

@@ -589,6 +587,166 @@ export type ResolvedConfig = Readonly<
  } & PluginHookUtils
 >

+// inferred ones are omitted
+export const configDefaults = Object.freeze({


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

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.

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.

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 🤔

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

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.

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

I guess given:

We couldn't expose all defaults as some are inferred.

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))

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.

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)

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

sapphi-red · 2024-11-01T09:05:22Z

packages/vite/src/node/config.ts

+      ? configDefaults.resolve.conditions.filter((c) => c !== 'node')
+      : configDefaults.resolve.conditions.filter((c) => c !== 'browser')


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

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?

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.

… them separately

sapphi-red · 2024-11-06T02:13:18Z

packages/vite/src/node/index.ts

@@ -7,6 +7,7 @@ export {
  loadConfigFromFile,
  resolveConfig,
  sortUserPlugins,
+  configDefaults,


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.

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 · 2024-11-08T02:55:04Z

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 · 2024-11-08T07:10:58Z

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

yannbf · 2024-11-08T07:25:40Z

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 · 2024-11-08T07:32:40Z

Open in Stackblitz

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

commit: e4364e5

It was always overriding the value

sapphi-red · 2024-11-08T10:03:07Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-11-08T10:20:17Z

📝 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 · 2024-11-08T10:22:54Z

It should be fine now.

patak-dev

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

packages/vite/src/node/utils.ts

bluwy · 2024-11-12T06:47:44Z

packages/vite/src/node/utils.ts

+  if (typeof value === 'function') {
+    return value as T
+  }
+  return structuredClone(value)


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.

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

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.

wip: configDefaults

ecc55a9

sapphi-red marked this pull request as draft November 1, 2024 09:03

sapphi-red commented Nov 1, 2024

View reviewed changes

sapphi-red changed the title ~~wip: configDefaults~~ wip: expose configDefaults Nov 1, 2024

sapphi-red changed the title ~~wip: expose configDefaults~~ feat: expose configDefaults Nov 1, 2024

sapphi-red added 3 commits November 5, 2024 14:43

refactor: define mergeWithDefaults and use it

6bb4258

refactor: use mergeWithDefaults in more places

09714d3

refactor: use constants

a9e7b6b

patak-dev added this to the 6.0 milestone Nov 5, 2024

sapphi-red added 2 commits November 6, 2024 10:43

chore: merge main

d84efb7

feat: remove mainFields and conditions from configDefaults and expose…

d6b5a30

… them separately

sapphi-red commented Nov 6, 2024

View reviewed changes

sapphi-red marked this pull request as ready for review November 6, 2024 02:13

sapphi-red added 2 commits November 7, 2024 11:30

chore: don't expose anything

c92253c

chore: merge main

7227dc2

sapphi-red changed the title ~~feat: expose configDefaults~~ refactor: introduce mergeWithDefaults and organize how default values for config options are set Nov 7, 2024

sapphi-red added the p1-chore Doesn't change code behavior (priority) label Nov 7, 2024

This comment was marked as outdated.

Sign in to view

patak-dev previously approved these changes Nov 7, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

sapphi-red added 6 commits November 8, 2024 08:22

refactor: make resolvedConfig.preview ResolvedPreviewOptions

dba5d65

fix: don't set default value of server.host

fd4ccb9

test: use toStrictEqual

f1e2db8

fix: use clone build options for compat

a5e77f0

refactor: extract builderOptionsDefaults

d9ef910

refactor: clone defaults

3313e83

sapphi-red dismissed patak-dev’s stale review via 3313e83 November 8, 2024 00:51

This comment was marked as outdated.

Sign in to view

This comment was marked as duplicate.

Sign in to view

fix: don't set emptyOutDir

6a52f2f

This comment was marked as outdated.

Sign in to view

hi-ogawa added the trigger: preview label Nov 8, 2024

sapphi-red added 2 commits November 8, 2024 18:50

fix: don't set emptyOutDir

a163661

It was always overriding the value

refactor: remove inferred value from buildEnvironmentOptionsDefaults

a2705e9

patak-dev previously approved these changes Nov 11, 2024

View reviewed changes

bluwy reviewed Nov 12, 2024

View reviewed changes

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved

sapphi-red added 2 commits November 12, 2024 15:20

chore: merge main

72ccc7d

perf: clone only once

ffb92fd

sapphi-red dismissed patak-dev’s stale review via ffb92fd November 12, 2024 06:27

fix: I thought I replaced it 😅

2d6bc9c

bluwy reviewed Nov 12, 2024

View reviewed changes

sapphi-red added 2 commits November 12, 2024 16:14

perf: avoid structuredClone

38162a0

fix: skip null and RegExp

e4364e5

bluwy approved these changes Nov 12, 2024

View reviewed changes

patak-dev merged commit 0e1f437 into vitejs:main Nov 12, 2024
16 checks passed

		? configDefaults.resolve.conditions.filter((c) => c !== 'node')
		: configDefaults.resolve.conditions.filter((c) => c !== 'browser')

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

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

Conversation

sapphi-red commented Nov 1, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

patak-dev left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

sapphi-red commented Nov 8, 2024 • edited Loading

This comment was marked as duplicate.

sapphi-red commented Nov 8, 2024

This comment was marked as outdated.

This comment was marked as outdated.

yannbf commented Nov 8, 2024

pkg-pr-new bot commented Nov 8, 2024 • edited Loading

sapphi-red commented Nov 8, 2024

vite-ecosystem-ci bot commented Nov 8, 2024

sapphi-red commented Nov 8, 2024

patak-dev left a comment

Choose a reason for hiding this comment

bluwy Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluwy left a comment

Choose a reason for hiding this comment

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

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

sapphi-red commented Nov 1, 2024 •

edited

Loading

sapphi-red commented Nov 8, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 8, 2024 •

edited

Loading

bluwy Nov 12, 2024 •

edited

Loading