-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
feat(ssr): resolve externalized packages with resolve.externalConditions and add module-sync to default external condition
#20409
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.
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
Conversation
vitest.config.e2e.ts
Outdated
| ssr: { | ||
| resolve: { | ||
| // FIXME: this is a bug in Vitest, resolving externalized modules should use externalConditions | ||
| conditions: ['node', 'module-sync'], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hi-ogawa It seems Vitest uses resolve.conditions for both externalized files and no-externalized files. Is it possible to configure Vitest to only use module-sync for externalized files? (since module-sync should not be used for no-externalized files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, currently Vitest (technically due to vite-node) cannot use resolve.externalConditions for server external resolution and only uses resolve.conditions for everything. I haven't followed yet but @sheremet-va might know better what will change in vitest-dev/vitest#8208.
Is it possible to configure Vitest to only use module-sync for externalized files?
What's the desired goal for this diff specifically? Should tests pass without adding this but something failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting the tests to pass without specifying ssr.resolve.conditions: ['node', 'module-sync'].
playground/ssr-resolve/main.js is bundled to playground/ssr-resolve/dist/main.js and that file has import moduleSync from '@vitejs/test-module-sync'. That bundled file is imported at this line.
| await expect(import(`${testDir}/dist/main.mjs`)).resolves.toBeTruthy() |
Since
@vitejs/test-module-sync is not processed by Vite / Vitest, I expected the resolver to use module-sync condition (as it's included in ssr.resolve.externalConditions). But it used the ssr.resolve.conditions and that does not include module-sync condition. So the resolution fails (pkg-module-sync only has module-sync condition).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No ideal but this is how it works currently. import(`${testDir}/dist/main.mjs`) is transformed and evaluated through vite-node and each package's external check is done after resolved through resolve.conditions (basically pluginContainer.resolveId) and it's not a same behavior as Vite SSR external. So, from what I know, this is not possible.
Is it possible to configure Vitest to only use module-sync for externalized files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure the purpose of this test. Is this to confirm Vitest's behavior (with current PR change) works same as Vite SSR? Or do you want to test importing dist/main.js on a pure node runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vitest has its own externalization logic because Vite only supports externalizing in SSR, but we need the same behavior for JSDOM. We resolve the ID with pluginContainer.resolveId and check if it needs to be external
We cannot use externalConditions because we don't know if it's external yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for the answers.
Is this to confirm Vitest's behavior (with current PR change) works same as Vite SSR? Or do you want to test importing
dist/main.json a pure node runtime?
The main purpose of the test was to confirm the built file works in pure Node. But since the test was written in this way, I encountered this and wondered whether it's a way to configure Vitest to work more similar to Node on this aspect. I can run node command instead of this dynamic import here.
We cannot use externalConditions because we don't know if it's external yet
I think Vite has the same problem. Vite uses externalConditions when checking whether it needs to be external.
The reasoning is #14498 (comment)
module-sync to default external condition
| import pkgExportsEntry from '@vitejs/test-resolve-pkg-exports/entry' | ||
| import deepFoo from '@vitejs/test-deep-import/foo' | ||
| import deepBar from '@vitejs/test-deep-import/bar' | ||
| // import deepBar from '@vitejs/test-deep-import/bar' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented out this test case as it wasn't working.
The output contains @vitejs/test-deep-import/bar as-is, but to make that work in Node, it needs to be rewritten to @vitejs/test-deep-import/utils/bar.js as Node does not read package.json for directory specifiers in ESM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean this could be a breaking change if someone relied on the non-standard behavior before?
I'm ok with trying this out in a minor, but I think if we're not going to support this case here we can directly remove the test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this code was not working without the change in this PR.
I changed the test code to run it in Node for #20409 (comment) and that caused the test to fail.
https://github.com/vitejs/vite/pull/20409/files#diff-859b1585f9b836443382acf98d7530dd7666ca2e0931021e82f745af51e233a3L26-R32
| const rawConditions = externalize | ||
| ? options.externalConditions | ||
| : options.conditions | ||
| const conditions = rawConditions.map((condition) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
If it is determined as external here,
vite/packages/vite/src/node/plugins/resolve.ts
Lines 348 to 352 in e7c3d47
| const external = | |
| options.externalize && | |
| options.isBuild && | |
| currentEnvironmentOptions.consumer === 'server' && | |
| shouldExternalize(this.environment, id, importer) |
the resolution here should use
externalConditions.| (res = tryNodeResolve(id, importer, options, depsOptimizer, external)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, it's only for build? Might not be relevant to this PR specifically but curious what's the idea for options.isBuild check? Is this because Vite crawl imports by ourselves by import analysis resolve (and not resolve for external) during dev, but this is specifically for rollup?
If we have externalize flag in resolveExportsOrImports, then technically can we also pass this flag instead of swapping off options.conditions for fetchModule node resolve?
vite/packages/vite/src/node/ssr/fetchModule.ts
Lines 51 to 54 in 8033e5b
| const resolved = tryNodeResolve(url, importer, { | |
| mainFields: ['main'], | |
| conditions: externalConditions, | |
| externalConditions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, it's only for build? Might not be relevant to this PR specifically but curious what's the idea for
options.isBuildcheck? Is this because Vite crawl imports by ourselves by import analysis resolve (and not resolve for external) during dev, but this is specifically for rollup?
It is only for build. The purpose is to run this part of code:
vite/packages/vite/src/node/plugins/resolve.ts
Lines 771 to 787 in e7c3d47
| let resolvedId = id | |
| if ( | |
| deepMatch && | |
| !pkg.data.exports && | |
| path.extname(id) !== path.extname(resolved.id) | |
| ) { | |
| // id date-fns/locale | |
| // resolve.id ...date-fns/esm/locale/index.js | |
| const index = resolved.id.indexOf(id) | |
| if (index > -1) { | |
| resolvedId = resolved.id.slice(index) | |
| debug?.( | |
| `[processResult] ${colors.cyan(id)} -> ${colors.dim(resolvedId)}`, | |
| ) | |
| } | |
| } | |
| return { ...resolved, id: resolvedId, external: true } |
In dev, this specifier rewrite is not needed because
fetchModule handles that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have
externalizeflag inresolveExportsOrImports, then technically can we also pass this flag instead of swapping offoptions.conditionsforfetchModulenode resolve?
No, we cannot do that. Because tryNodeResolve will return a non-absolute id when externalize is true (e.g. react/jsx-runtime).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference 👍 As I understand, the latter part of processResult where it checks resolved.id is fairly an edge case, so previously using conditions instead of externalConditions wouldn't have made much difference.
I'm still digesting the history of that logic, but will follow up on that on my own.
| const rawConditions = externalize | ||
| ? options.externalConditions | ||
| : options.conditions | ||
| const conditions = rawConditions.map((condition) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference 👍 As I understand, the latter part of processResult where it checks resolved.id is fairly an edge case, so previously using conditions instead of externalConditions wouldn't have made much difference.
I'm still digesting the history of that logic, but will follow up on that on my own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC the PR also includes a bug fix for resolving externaled packages. I think it would be nice if it can be mentioned in the PR title too so it's tracked in the changelog in case it affects someone.
I'm a bit afraid that this could break for some people, so maybe something we need to watch out for after merging in case we should reconsider it.
module-sync to default external conditionresolve.externalConditions add module-sync to default external condition
resolve.externalConditions add module-sync to default external conditionresolve.externalConditions and add module-sync to default external condition
|
/ecosystem-ci run |
commit: |
This comment was marked as outdated.
This comment was marked as outdated.
|
📝 Ran ecosystem CI on
✅ ladle, analogjs, marko, one, qwik, react-router, laravel, nuxt, quasar, sveltekit, storybook, rakkas, vite-environment-examples, unocss, vite-plugin-react, vite-plugin-svelte, vite-plugin-pwa, vite-plugin-cloudflare, vite-setup-catalogue, histoire, vite-plugin-vue, vuepress, vite-plugin-rsc, vitepress |
|
waku is failing with the main branch as well: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/16592408110/job/46931036774 |
….externalConditions` (#5491) Port of vitejs/vite#20409
Description
built on top of #20279This PR adds
module-syncto the defaultresolve.externalCondition.module-synccondition is a condition introduced by Node when it supported require(ESM). This condition is used by Node when resolving bothrequireandimportwhen require(ESM) is enabled.All Node versions supported by Vite supports require(ESM), so for
ssrLoadModuleand RunnableDevEnvironment, this change is fine. For other DevEnvironments that runs on non-Node runtime or Node version that doesn't support require(ESM), it would need to setresolve.externalConditionif they would like to align with the runtime's behavior. (It is not possible to set the conditions on our side as Vite doesn't know which runtime the environment will run on)close #19201