feat(resolve)!: allow removing conditions

[sapphi-red]

Description

Updates the default conditions / externalConditions and remove the need for overrideConditions and other odd logics.

refs #18016
refs #18332
refs #17326
refs #9860

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

I'm using webCompatible here for now. But probably we should do it in the env side. I left this for now to do that when removing the webCompatible flag. I think it would be easier to understand the diff later if it's done at the same time with the browser field.

[sapphi-red]

Update: I removed webCompatible in this PR (edcf8f3)

[sapphi-red]

I didn't give users a way to remove require / import as I didn't understand how that would work.

[sapphi-red]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 15f4875: Open

suite	result	latest scheduled
astro	❌ failure	❌ failure
histoire	❌ failure	✅ success
quasar	❌ failure	✅ success
remix	❌ failure	❌ failure
sveltekit	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-plugin-svelte	❌ failure	✅ success
vitest	❌ failure	❌ failure

✅ analogjs, ladle, laravel, marko, nuxt, previewjs, qwik, rakkas, redwoodjs, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

[sapphi-red]

If we change the default to ['module', 'browser', 'node', 'production', 'development'] (713cee3), the following CIs fail (#18395 (comment)):

• histoire: https://github.com/histoire-dev/histoire/blob/f5833710e22b8f6b55dd46693f3c80c4ad807c80/packages/histoire/src/node/vite.ts#L100
• quasar: If I add environments.client.webCompatible to https://github.com/quasarframework/quasar/blob/70d04f5059312be732e50b2a65cc79716e9bcbac/vite-plugin/testing/runtime/vitest.config.js#L32, it passes
• vite-plugin-svelte: https://github.com/sveltejs/vite-plugin-svelte/blob/52e4a4f165dea8442ec729d73cd43f1aef2eed08/packages/vite-plugin-svelte/src/utils/options.js#L350
• vitest: https://github.com/vitest-dev/vitest/blob/9d9bad5b1025576bbd58cc67db39c3e4afaf7329/packages/vitest/src/node/pool.ts#L93-L97

This behavior aligns with resolve.mainFields, but it's not easy to remove default conditions.

import { configDefaults } from 'vite'

const plugin = {
  name: 'remove-conditions',
  config(config) {
    config.resolve ||= {}
    config.resolve.conditions ||= configDefaults.resolve.conditions
    config.resolve.conditions = config.resolve.conditions.filter(
      (c) => c !== 'node',
    )
  }
}

If we put the default conditions before config hooks (c8ab916), it will be easier to remove the defaults. But more CI failed 😅 (#18395 (comment)).

I'll revert to the original one (713cee3).

[sapphi-red]

Pushed a commit that removes webCompatible. Let's see if that reduces the fails.

[sapphi-red]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on edcf8f3: Open

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

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

[patak-dev]

What is a good use case for not passing production and development?

I'm wondering if we should treat these two specially, like you do with import, require, default. Given that they are the only ones that are conditionally applied, it feels cleaner for resolve.conditions to be "the conditions that are always applied". We could add a way to pass a function for each condition to specify
the conditions for example if we want to explicitly define them, or ask for
['module', 'browser', 'node', process.env.NODE_ENV], maybe this should be the default if we keep it in the conditions?)

[sapphi-red]

As we discussed in the team meeting, I introduced DEV_PROD_CONDITION variable that will be replaced with production or development: 4985011

[sapphi-red]

I don't know why but without this change, the test fails.

[sapphi-red]

I have updated to be the behavior decided in the meeting.

[patak-dev]

I would prefer we just use a string to be honest. I think the SpecialCondition is the right approach but it forces users to import it to use the option (that is the most common thing to do). If we want to add a special condition, maybe we could just add resolve.conditionDevProd: boolean defaulting to true.
If not, I think we could use a string like vite-dev-prod and there shouldn't be any chance or collision. But thinking a bit more in the API, I'm now leaning towards a new resolve.conditionDevProd option because vite-dev-prod will always be a footgun with a lot of users forgetting to add it. It makes the resolve.conditions option a lot trickier to use for the common case.

[bluwy]

In the meeting I suggested that maybe we should keep using "production" and "development" still and avoid a magic string. If one of them is added, the other must exist or it'll warn (we'll auto add it anyways). That way both conditions will always exist. Perhaps this is a simpler approach so we don't have to introduce a new option?

[patak-dev]

Don't you think that a lot of people will forget to add , "production", "development"? If 99% of the time we need them there, I think resolve.conditionDevProd: boolean would lead to a better experience

[bluwy]

I don't know if it'll be a common thing for people to do. IIUC we have the condition by default, so those who change the conditions are probably plugin/env authors so they'll be able to address this early on. And if we emit a warning, it should nudge the authors to the right direction without too much hassle I feel like 🤔

[patak-dev]

You can't emit a warning if people don't add production+development as that would be fine. Oh, you are saying that we can emit a warning if only one is added. What I'm worried about is that a lot of times these won't end up added. I prefer your idea to the custom symbol though, but I would still prefer the boolean. Curious to see what others think.

[sapphi-red]

I'm fine with changing the string value to something that isn't that strange to write it directly in the config as long as it indicates it's a special value (It still works now when you write vite$"dev-prod" in the config directly instead of importing the variable). To raise a few ideas: @vite-dev-prod / _vite-dev-prod / vite:dev-prod / @vite:dev-prod.

vite-dev-prod will always be a footgun with a lot of users forgetting to add it.

I think this is not special to vite-dev-prod. All module, node, browser might be forgot to add by users. I think configDefaults would help in this case (at least not to forget updating the code when the default changes in future).

[patak-dev]

What about just vite_env, in reference to NODE_ENV?

[sapphi-red]

For me, vite_env feels like it will be replaced with NODE_ENV which is not the case here.

[patak-dev]

Ok, vite:dev-prod sounds ok. Other options development|production, dev|prod

[bluwy]

LGTM. Makes sense to me to follow-up with a way to expose configDefaults or something too.

[patak-dev]

This will be huge for a cleaner future, thanks for pushing it through @sapphi-red

[dario-piotrowicz]

I think this change is not correct, since there might be webCompatible servers such as Cloudflare's workerd

[sapphi-red]

linking for future reference: #18611