fix(resolve): always use `module` condition #13370

Andarist · 2023-05-29T19:12:09Z

Description

In this PR I remove filtering out of the module condition. It's a condition specifically meant to help package authors have their packages consumable using require and import in bundlers while avoiding the dual package hazard.

Additional context

@garronej has found this bug in Vite here: https://github.com/garronej/vite-dual-package-repro-repo . This affects Emotion and all UI libraries built on top of it.

How Vite handles this right now is inconsistent with other bundlers (such as webpack, Rollup with its @rollup/plugin-node-resolve, bun, and esbuild).

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 PR Title 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.

cc @bluwy @patak-dev

fixes #11389

stackblitz · 2023-05-29T19:12:11Z

Run & review this pull request in StackBlitz Codeflow.

Andarist · 2023-05-30T07:48:30Z

playground/resolve/index.html

+  import { msg as exportsWithModuleCondition } from '@vitejs/test-resolve-exports-with-module-condition'
+  import { msg as exportsWithModuleConditionRequired } from '@vitejs/test-resolve-exports-with-module-condition-required'


This fails when running pnpm run test-build but it works with cd playground/resolve && pnpm run dev (thanks to adding @vitejs/test-resolve-exports-with-module-condition-required to optimizeDeps here). How should I configure the actual test script to do the right thing here (to process linked CJS modules)?

If the dep needs to be optimized during build time, you could set optimizeDeps.disabled: false in the config (the default is disabled 'build'). But I think we may need to move these tests to the optimize-deps playground though as we aren't yet testing deps optimization at build for all playgrounds (it is an experimental feature).

What I don't understand though is the fact that there are some other CJS modules in playground/resolve. So it seems like should be possible to keep this test here (I feel like it belongs to this particular test suite, otoh - if that's problematic in the current setup... it's definitely not something worth fighting for). Do you happen to know what's the difference between those other tests and this one?

It was only if you needed the dependency to be optimized during build time. We're currently only testing deps optimization at build time in the optimize-deps playground (or when using the test-build-without-plugin-commonjs script). By default deps aren't optimized during build and plugin commonjs is used to do interop.

Hm, so I don't really want to get those deps optimized anyhow (I think?), i'd only like to apply commonjs interop here. Or is the 'module' condition handling part of the optimizations in Vite?

It works fine during build if you use index.cjs instead of a .js file in the @vitejs/test-resolve-exports-with-module-condition-required. This is also how other CJS packages are constructed in the resolve playground. I thought yours should work too though.

Thanks for the tip. I changed the extension and it seems to work now - the CI should pass shortly

Another way is to use the file: protocol like "file:./exports-with-module-condition-required" in the package.json so it's hardlinked instead, but either way also works fine.

patak-dev · 2023-05-30T15:42:19Z

@sapphi-red would you check this one when you have some time? If I recall correctly you also wanted to apply this change.

bluwy

LGTM! I've been confused of how the module condition work because the webpack docs doesn't really explain it well. I didn't know esbuild supports it too, and as usual it has excellent docs 😄

patak-dev · 2023-05-31T06:03:16Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-05-31T06:03:43Z

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	✅ success
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	❌ failure
nuxt	❌ failure
nx	✅ success
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
unocss	✅ success
vite-plugin-pwa	✅ 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	✅ success

bluwy · 2023-05-31T06:14:28Z

Would this also fix #11389?

Andarist · 2023-05-31T06:42:36Z

didn't know esbuild supports it too, and as usual it has excellent docs 😄

Note that I'd consider part of those docs to indicate a problem with esbuild: "The following condition is also automatically included when the platform is set to either browser or node and no custom conditions are configured.". The module condition shouldn't be specific to the used platform - it just should be enabled at all times (unless you override it somehow)

It seems though that the only current choices for the platform are node, browser and neutral. Neutral is supposed to be "please configure everything yourself". So I guess that this is somewhat OK right now but I'd consider the wording to be misleading as other projects might assume incorrect semantics based on that sentence.

Relevant code in esbuild: https://github.com/evanw/esbuild/blob/e6a8169c3a574f4c67d4cdd5f31a938b53eb7421/pkg/api/api_impl.go#L1409-L1412

/ecosystem-ci run

Should I be worried about the failures here? or are those flaky?

Would this also fix #11389?

Yes. I verified this manually and marked this PR as closing that issue.

patak-dev · 2023-05-31T06:53:19Z

/ecosystem-ci run

Should I be worried about the failures here? or are those flaky?

Marko and Nuxt are also failing in main right now. Qwik is green there though https://github.com/vitejs/vite-ecosystem-ci/actions/runs/5129068924/jobs/9226347832

I'll re-run qwik now. We're about to start the beta for 4.4, so maybe we could merge it there.

patak-dev

Qwik was a glitch, it is green after re-running. I think we could still include this in a patch for 4.3. @sapphi-red what do you think?

sapphi-red

Thanks 💚

This current behavior has existed for a long time, so I vote to wait for Vite 4.4 just to be sure: 39820b9#diff-9b81bb364c02eab9494a7d27a5effc400cacbffd3b8f349c192f890c37bfc83fL543-R560

Andarist mentioned this pull request May 29, 2023

[ESM] Feature a distribution compatible with Node in "type": "module". mui/material-ui#37335

Open

2 tasks

Andarist force-pushed the fix-module-condition branch 3 times, most recently from da0652e to 594a48f Compare May 30, 2023 07:41

Andarist commented May 30, 2023

View reviewed changes

fix(resolve): always use module condition

716ca9a

Andarist force-pushed the fix-module-condition branch from 594a48f to 716ca9a Compare May 30, 2023 22:52

bluwy previously approved these changes May 31, 2023

View reviewed changes

bluwy added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label May 31, 2023

patak-dev previously approved these changes May 31, 2023

View reviewed changes

sapphi-red previously approved these changes May 31, 2023

View reviewed changes

patak-dev added this to the 4.4 milestone May 31, 2023

chore: merge main

1d002cb

patak-dev dismissed stale reviews from sapphi-red, bluwy, and themself via 1d002cb June 6, 2023 19:40

patak-dev merged commit 367920b into vitejs:main Jun 7, 2023
12 of 13 checks passed

sapphi-red mentioned this pull request Sep 20, 2023

Tests failing with MUI theme interface customization after vite upgrade #14176

Closed

7 tasks

garronej added a commit to garronej/ts-ci that referenced this pull request Oct 8, 2023

https://github.com/vitejs/vite/pull/13370

e410445

garronej mentioned this pull request Nov 17, 2023

TssCacheProvider is not working when used with emotion cache and mui 5 garronej/tss-react#201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolve): always use `module` condition #13370

fix(resolve): always use `module` condition #13370

Andarist commented May 29, 2023 •

edited

stackblitz bot commented May 29, 2023

Andarist May 30, 2023

patak-dev May 30, 2023

Andarist May 30, 2023

patak-dev May 30, 2023

Andarist May 30, 2023

patak-dev May 30, 2023

Andarist May 30, 2023

bluwy May 31, 2023

patak-dev commented May 30, 2023

bluwy left a comment

patak-dev commented May 31, 2023

vite-ecosystem-ci bot commented May 31, 2023 •

edited

bluwy commented May 31, 2023

Andarist commented May 31, 2023 •

edited

patak-dev commented May 31, 2023

patak-dev left a comment

sapphi-red left a comment

		import { msg as exportsWithModuleCondition } from '@vitejs/test-resolve-exports-with-module-condition'
		import { msg as exportsWithModuleConditionRequired } from '@vitejs/test-resolve-exports-with-module-condition-required'

fix(resolve): always use module condition #13370

fix(resolve): always use module condition #13370

Conversation

Andarist commented May 29, 2023 • edited

Description

Additional context

What is the purpose of this pull request?

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

stackblitz bot commented May 29, 2023

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

patak-dev commented May 30, 2023

bluwy left a comment

Choose a reason for hiding this comment

patak-dev commented May 31, 2023

vite-ecosystem-ci bot commented May 31, 2023 • edited

bluwy commented May 31, 2023

Andarist commented May 31, 2023 • edited

patak-dev commented May 31, 2023

patak-dev left a comment

Choose a reason for hiding this comment

sapphi-red left a comment

Choose a reason for hiding this comment

fix(resolve): always use `module` condition #13370

fix(resolve): always use `module` condition #13370

Andarist commented May 29, 2023 •

edited

vite-ecosystem-ci bot commented May 31, 2023 •

edited

Andarist commented May 31, 2023 •

edited