Skip to content

Conversation

@sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Nov 14, 2022

Description

At the time I created #10683, I was going to propose changing resolve.conditions's default to ['production', 'development', 'module', 'node', 'browser', 'import', 'require'].
This way people can remove default conditions. (The implementation will be like overrideConditions that I implemented in #10683)

But I realized there's a problems with this.
It's not possible to make #9860 work. Even if resolve.conditions was set to ['node', 'import', 'require'], node condition won't be used for targetWeb.

This PR just cleans the logic around this as I didn't come up with a solution.

refs #10683

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the p1-chore Doesn't change code behavior (priority) label Nov 14, 2022
Comment on lines 1054 to 1069
const conditions = [...additionalConditions].filter((condition) => {
switch (condition) {
case 'production':
return options.isProduction
case 'development':
return !options.isProduction
case 'module':
return !options.isRequire
}
return true
})

return _resolveExports(pkg, key, {
browser: targetWeb && !conditions.includes('node'),
require: options.isRequire && !conditions.includes('import'),
browser: targetWeb && !additionalConditions.has('node'),
require: options.isRequire && !additionalConditions.has('import'),
conditions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  const conditions = [...additionalConditions].filter((condition) => {
    switch (condition) {
      case 'production':
        return options.isProduction
      case 'development':
        return !options.isProduction
      case 'import':
      case 'module':
        return !options.isRequire
      case 'require':
        return options.isRequire
    }
    return true
  })

  return _resolveExports(pkg, key, {
    conditions,
    unsafe: true

This is what I was going to propose.

@aleclarson

This comment was marked as outdated.

@sapphi-red sapphi-red marked this pull request as ready for review April 21, 2023 11:03
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Apr 21, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure
windicss ✅ success

@patak-dev patak-dev merged commit ad21ec3 into vitejs:main Apr 21, 2023
@sapphi-red sapphi-red deleted the refactor/resolve-conditions branch April 22, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p1-chore Doesn't change code behavior (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants