Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(resolve): always use module condition #13370

Merged
merged 2 commits into from
Jun 7, 2023
Merged

fix(resolve): always use module condition #13370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
716ca9a
fix(resolve): always use `module` condition
Andarist May 29, 2023
1d002cb
chore: merge main
patak-dev Jun 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/vite/src/node/plugins/resolve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1105,8 +1105,6 @@ function resolveExportsOrImports(
return options.isProduction
case 'development':
return !options.isProduction
case 'module':
return !options.isRequire
}
return true
})
Expand Down
9 changes: 9 additions & 0 deletions playground/resolve/__tests__/resolve.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ test('Respect exports to take precedence over mainFields', async () => {
expect(await page.textContent('.exports-with-module')).toMatch('[success]')
})

test('import and require resolve using module condition', async () => {
expect(await page.textContent('.exports-with-module-condition')).toMatch(
'[success]',
)
expect(
await page.textContent('.exports-with-module-condition-required'),
).toMatch('[success]')
})

test('implicit dir/index.js', async () => {
expect(await page.textContent('.index')).toMatch('[success]')
})
Expand Down
3 changes: 3 additions & 0 deletions playground/resolve/exports-with-module-condition-required/index.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/* eslint-disable import/no-commonjs */
const { msg } = require('@vitejs/test-resolve-exports-with-module-condition')
module.exports = { msg }
9 changes: 9 additions & 0 deletions playground/resolve/exports-with-module-condition-required/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "@vitejs/test-resolve-exports-with-module-condition-required",
"private": true,
"version": "1.0.0",
"main": "index.cjs",
"dependencies": {
"@vitejs/test-resolve-exports-with-module-condition": "link:../exports-with-module-condition"
}
}
1 change: 1 addition & 0 deletions playground/resolve/exports-with-module-condition/index.esm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const msg = '[success] exports with module condition (index.esm.js)'
2 changes: 2 additions & 0 deletions playground/resolve/exports-with-module-condition/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/* eslint-disable import/no-commonjs */
module.exports.msg = '[fail] exports with module condition (index.js)'
1 change: 1 addition & 0 deletions playground/resolve/exports-with-module-condition/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const msg = '[fail] exports with module condition (index.mjs)'
10 changes: 10 additions & 0 deletions playground/resolve/exports-with-module-condition/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "@vitejs/test-resolve-exports-with-module-condition",
"private": true,
"version": "1.0.0",
"exports": {
"module": "./index.esm.js",
"import": "./index.mjs",
"require": "./index.js"
}
}
15 changes: 15 additions & 0 deletions playground/resolve/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ <h2>Exports with legacy fallback</h2>
<h2>Exports with module</h2>
<p class="exports-with-module">fail</p>

<h2>
Both import and require resolve using module condition (avoids dual package
hazard)
</h2>
<p class="exports-with-module-condition">fail</p>
<p class="exports-with-module-condition-required">fail</p>

<h2>Resolving top level with imports field</h2>
<p class="imports-top-level">fail</p>

Expand Down Expand Up @@ -214,6 +221,14 @@ <h2>resolve package that contains # in path</h2>
import { msg as exportsWithModule } from '@vitejs/test-resolve-exports-with-module'
text('.exports-with-module', exportsWithModule)

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'
Comment on lines +224 to +225
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

text('.exports-with-module-condition', exportsWithModuleCondition)
text(
'.exports-with-module-condition-required',
exportsWithModuleConditionRequired,
)

// imports field
import { msg as importsTopLevel } from '#top-level'
text('.imports-top-level', importsTopLevel)
Expand Down
2 changes: 2 additions & 0 deletions playground/resolve/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
"@vitejs/test-resolve-exports-legacy-fallback": "link:./exports-legacy-fallback",
"@vitejs/test-resolve-exports-path": "link:./exports-path",
"@vitejs/test-resolve-exports-with-module": "link:./exports-with-module",
"@vitejs/test-resolve-exports-with-module-condition": "link:./exports-with-module-condition",
"@vitejs/test-resolve-exports-with-module-condition-required": "link:./exports-with-module-condition-required",
"@vitejs/test-resolve-linked": "workspace:*",
"@vitejs/test-resolve-imports-pkg": "link:./imports-path/other-pkg",
"@vitejs/test-resolve-sharp-dir": "link:./sharp-dir"
Expand Down
1 change: 1 addition & 0 deletions playground/resolve/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export default defineConfig({
],
optimizeDeps: {
include: [
'@vitejs/test-resolve-exports-with-module-condition-required',
'@vitejs/test-require-pkg-with-module-field',
'@vitejs/test-resolve-sharp-dir',
],
Expand Down
30 changes: 16 additions & 14 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.