Skip to content

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.

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

Merged
sapphi-red merged 7 commits into from
Jul 29, 2025
Merged

feat(ssr): resolve externalized packages with resolve.externalConditions and add module-sync to default external condition #20409

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3706eee
feat(exports): export `defaultExternalConditions`
Andarist Jun 24, 2025
87d34b9
feat(ssr): add `module-sync` to default `externalCondition`
sapphi-red Jul 15, 2025
1e2fc8f
wip
sapphi-red Jul 15, 2025
26b1b82
chore: update jsdoc comment
sapphi-red Jul 17, 2025
6d38029
chore: merge main
sapphi-red Jul 25, 2025
7ad1b12
test: use node command
sapphi-red Jul 25, 2025
e7c3d47
test: comment out failing test case that wasn't working
sapphi-red Jul 25, 2025
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
3 changes: 2 additions & 1 deletion packages/vite/src/node/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
DEFAULT_CLIENT_CONDITIONS,
DEFAULT_CLIENT_MAIN_FIELDS,
DEFAULT_CONFIG_FILES,
DEFAULT_EXTERNAL_CONDITIONS,
DEFAULT_PREVIEW_PORT,
DEFAULT_SERVER_CONDITIONS,
DEFAULT_SERVER_MAIN_FIELDS,
Expand Down Expand Up @@ -651,7 +652,7 @@ export const configDefaults = Object.freeze({
resolve: {
// mainFields
// conditions
externalConditions: ['node'],
externalConditions: [...DEFAULT_EXTERNAL_CONDITIONS],
extensions: ['.mjs', '.js', '.mts', '.ts', '.jsx', '.tsx', '.json'],
dedupe: [],
/** @experimental */
Expand Down
5 changes: 5 additions & 0 deletions packages/vite/src/node/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ export const DEFAULT_SERVER_CONDITIONS = Object.freeze(
DEFAULT_CONDITIONS.filter((c) => c !== 'browser'),
)

export const DEFAULT_EXTERNAL_CONDITIONS = Object.freeze([
'node',
'module-sync',
])

/**
* The browser versions that are included in the Baseline Widely Available on 2025-05-01.
*
Expand Down
1 change: 1 addition & 0 deletions packages/vite/src/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export {
VERSION as version,
DEFAULT_CLIENT_CONDITIONS as defaultClientConditions,
DEFAULT_CLIENT_MAIN_FIELDS as defaultClientMainFields,
DEFAULT_EXTERNAL_CONDITIONS as defaultExternalConditions,
DEFAULT_SERVER_CONDITIONS as defaultServerConditions,
DEFAULT_SERVER_MAIN_FIELDS as defaultServerMainFields,
defaultAllowedOrigins,
Expand Down
26 changes: 22 additions & 4 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 @@ -753,7 +753,7 @@ export function tryNodeResolve(
const resolveId = deepMatch ? resolveDeepImport : resolvePackageEntry
const unresolvedId = deepMatch ? '.' + id.slice(pkgId.length) : id

let resolved = resolveId(unresolvedId, pkg, options)
let resolved = resolveId(unresolvedId, pkg, options, externalize)
if (!resolved) {
return
}
Expand Down Expand Up @@ -893,6 +893,7 @@ export function resolvePackageEntry(
id: string,
{ dir, data, setResolvedCache, getResolvedCache }: PackageData,
options: InternalResolveOptions,
externalize?: boolean,
): string | undefined {
const { file: idWithoutPostfix, postfix } = splitFileAndPostfix(id)

Expand All @@ -907,7 +908,13 @@ export function resolvePackageEntry(
// resolve exports field with highest priority
// using https://github.com/lukeed/resolve.exports
if (data.exports) {
entryPoint = resolveExportsOrImports(data, '.', options, 'exports')
entryPoint = resolveExportsOrImports(
data,
'.',
options,
'exports',
externalize,
)
}

// fallback to mainFields if still not resolved
Expand Down Expand Up @@ -987,8 +994,12 @@ function resolveExportsOrImports(
key: string,
options: InternalResolveOptions,
type: 'imports' | 'exports',
externalize?: boolean,
) {
const conditions = options.conditions.map((condition) => {
const rawConditions = externalize
? options.externalConditions
: options.conditions
const conditions = rawConditions.map((condition) => {
Comment on lines +1001 to +1004

@hi-ogawa hi-ogawa Jul 25, 2025

Copy link
Copy Markdown
Contributor

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?

@sapphi-red sapphi-red Jul 25, 2025

Copy link
Copy Markdown
Member Author

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.

vite/packages/vite/src/node/plugins/resolve.ts

Line 383 in e7c3d47

(res = tryNodeResolve(id, importer, options, depsOptimizer, external))

@hi-ogawa hi-ogawa Jul 26, 2025

Copy link
Copy Markdown
Contributor

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,

@sapphi-red sapphi-red Jul 28, 2025

Copy link
Copy Markdown
Member Author

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?

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.

@sapphi-red sapphi-red Jul 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

No, we cannot do that. Because tryNodeResolve will return a non-absolute id when externalize is true (e.g. react/jsx-runtime).

@hi-ogawa hi-ogawa Jul 28, 2025

Copy link
Copy Markdown
Contributor

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.

if (condition === DEV_PROD_CONDITION) {
return options.isProduction ? 'production' : 'development'
}
Expand All @@ -1010,6 +1021,7 @@ function resolveDeepImport(
id: string,
{ setResolvedCache, getResolvedCache, dir, data }: PackageData,
options: InternalResolveOptions,
externalize?: boolean,
): string | undefined {
const cache = getResolvedCache(id, options)
if (cache) {
Expand All @@ -1024,7 +1036,13 @@ function resolveDeepImport(
if (isObject(exportsField) && !Array.isArray(exportsField)) {
// resolve without postfix (see #7098)
const { file, postfix } = splitFileAndPostfix(relativeId)
const exportsId = resolveExportsOrImports(data, file, options, 'exports')
const exportsId = resolveExportsOrImports(
data,
file,
options,
'exports',
externalize,
)
if (exportsId !== undefined) {
relativeId = exportsId + postfix
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/ssr/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface SSROptions {
/**
* Conditions that are used during ssr import (including `ssrLoadModule`) of externalized dependencies.
*
* @default []
* @default ['node']
Comment thread
sapphi-red marked this conversation as resolved.
Outdated
*/
externalConditions?: string[]

Expand Down
2 changes: 2 additions & 0 deletions playground/ssr-resolve/__tests__/ssr-resolve.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ test.runIf(isBuild)('correctly resolve entrypoints', async () => {
new RegExp(`from ${_}@vitejs/test-deep-import/bar${_}`),
)

expect(contents).toMatch(new RegExp(`from ${_}@vitejs/test-module-sync${_}`))

await expect(import(`${testDir}/dist/main.mjs`)).resolves.toBeTruthy()
})

Expand Down
2 changes: 2 additions & 0 deletions playground/ssr-resolve/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import fileEntry from '@vitejs/test-entries/file'
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 moduleSync from '@vitejs/test-module-sync'
import { used } from './util'

export default `
Expand All @@ -14,5 +15,6 @@ export default `
pkg-exports/entry: ${pkgExportsEntry}
deep-import/foo: ${deepFoo}
deep-import/bar: ${deepBar}
module-sync: ${moduleSync}
util: ${used(['[success]'])}
`
5 changes: 3 additions & 2 deletions playground/ssr-resolve/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
"debug": "node --inspect-brk ../../packages/vite/bin/vite build"
},
"dependencies": {
"@vitejs/test-deep-import": "file:./deep-import",
"@vitejs/test-entries": "file:./entries",
"@vitejs/test-resolve-pkg-exports": "file:./pkg-exports",
"@vitejs/test-deep-import": "file:./deep-import"
"@vitejs/test-module-sync": "file:./pkg-module-sync",
"@vitejs/test-resolve-pkg-exports": "file:./pkg-exports"
}
}
1 change: 1 addition & 0 deletions playground/ssr-resolve/pkg-module-sync/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 'module-sync'
11 changes: 11 additions & 0 deletions playground/ssr-resolve/pkg-module-sync/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@vitejs/test-module-sync",
"type": "module",
"private": true,
"version": "0.0.0",
"exports": {
".": {
"module-sync": "./index.js"
}
}
}
10 changes: 10 additions & 0 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.

6 changes: 6 additions & 0 deletions vitest.config.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ export default defineConfig({
},
},
},
ssr: {
resolve: {
// FIXME: this is a bug in Vitest, resolving externalized modules should use externalConditions
conditions: ['node', 'module-sync'],
},
},

@sapphi-red sapphi-red Jul 17, 2025

Copy link
Copy Markdown
Member Author

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)

@hi-ogawa hi-ogawa Jul 17, 2025

Copy link
Copy Markdown
Contributor

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?

@sapphi-red sapphi-red Jul 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

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.

vite/playground/ssr-resolve/__tests__/ssr-resolve.spec.ts

Line 28 in 26b1b82

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

@hi-ogawa hi-ogawa Jul 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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?

@hi-ogawa hi-ogawa Jul 17, 2025

Copy link
Copy Markdown
Contributor

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?

@sheremet-va sheremet-va Jul 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

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

@sapphi-red sapphi-red Jul 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

esbuild: {
target: 'node20',
},
Expand Down