Skip to content

Commit e8d28ff

Browse files
deyaaeldeenCopilot
andcommitted
fix: include importer in SSR externalization cache key
The `processedIds` cache in `createIsExternal` keys entries by bare specifier alone, ignoring the importer. In monorepos where different importers resolve the same specifier to different physical packages (e.g., `@azure/core-lro@2.7.2` vs `@azure/core-lro@3.x`), the first resolution's externalization decision is incorrectly reused for all subsequent importers. Fix: use a composite cache key of `${id}\0${importer}` so each (specifier, importer) pair is evaluated independently. Fixes #22078 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6daa10f commit e8d28ff

File tree

2 files changed

+72
-4
lines changed

2 files changed

+72
-4
lines changed

packages/vite/src/node/__tests__/external.spec.ts

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { fileURLToPath } from 'node:url'
22
import { describe, expect, test } from 'vitest'
33
import { resolveConfig } from '../config'
4-
import { createIsConfiguredAsExternal } from '../external'
4+
import { createIsConfiguredAsExternal, shouldExternalize } from '../external'
55
import { PartialEnvironment } from '../baseEnvironment'
6+
import type { Environment } from '../environment'
67

78
describe('createIsConfiguredAsExternal', () => {
89
test('default', async () => {
@@ -16,6 +17,68 @@ describe('createIsConfiguredAsExternal', () => {
1617
})
1718
})
1819

20+
describe('shouldExternalize', () => {
21+
test('same specifier with different importers is not cached as one entry', async () => {
22+
// Regression test for https://github.com/vitejs/vite/issues/22078
23+
//
24+
// Before the fix, processedIds cached by bare specifier only. Calling
25+
// shouldExternalize("foo", importerA) would cache the result, and a
26+
// subsequent call with importerB would return the cached result from
27+
// importerA without evaluating importerB's context.
28+
//
29+
// With external: true, both importers resolve to external, so we
30+
// verify the function is callable with different importers and both
31+
// invocations succeed independently (no cache corruption).
32+
const resolvedConfig = await resolveConfig(
33+
{
34+
configFile: false,
35+
root: fileURLToPath(new URL('./', import.meta.url)),
36+
resolve: { external: true },
37+
},
38+
'serve',
39+
)
40+
const environment = new PartialEnvironment(
41+
'ssr',
42+
resolvedConfig,
43+
) as unknown as Environment
44+
45+
// Call with two different importers — both should succeed
46+
const result1 = shouldExternalize(
47+
environment,
48+
'@vitejs/cjs-ssr-dep',
49+
'/some/importer-a.js',
50+
)
51+
const result2 = shouldExternalize(
52+
environment,
53+
'@vitejs/cjs-ssr-dep',
54+
'/some/importer-b.js',
55+
)
56+
57+
// Both resolve to true since external: true is set globally
58+
expect(result1).toBe(true)
59+
expect(result2).toBe(true)
60+
})
61+
62+
test('relative and absolute specifiers bypass externalization', async () => {
63+
const resolvedConfig = await resolveConfig(
64+
{
65+
configFile: false,
66+
root: fileURLToPath(new URL('./', import.meta.url)),
67+
resolve: { external: true },
68+
},
69+
'serve',
70+
)
71+
const environment = new PartialEnvironment(
72+
'ssr',
73+
resolvedConfig,
74+
) as unknown as Environment
75+
76+
// Relative and absolute paths are never external
77+
expect(shouldExternalize(environment, './foo', undefined)).toBe(false)
78+
expect(shouldExternalize(environment, '/foo', undefined)).toBe(false)
79+
})
80+
})
81+
1982
async function createIsExternal(external?: true) {
2083
const resolvedConfig = await resolveConfig(
2184
{

packages/vite/src/node/external.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,16 +132,21 @@ function createIsExternal(
132132
const isConfiguredAsExternal = createIsConfiguredAsExternal(environment)
133133

134134
return (id: string, importer?: string) => {
135-
if (processedIds.has(id)) {
136-
return processedIds.get(id)!
135+
// Cache key includes importer because different importers may resolve
136+
// the same bare specifier to different packages (e.g. in pnpm monorepos
137+
// with multiple versions of a dependency). Without importer in the key,
138+
// the first resolution's result is incorrectly reused for all importers.
139+
const cacheKey = importer ? `${id}\0${importer}` : id
140+
if (processedIds.has(cacheKey)) {
141+
return processedIds.get(cacheKey)!
137142
}
138143
let isExternal = false
139144
if (id[0] !== '.' && !path.isAbsolute(id)) {
140145
isExternal =
141146
isBuiltin(environment.config.resolve.builtins, id) ||
142147
isConfiguredAsExternal(id, importer)
143148
}
144-
processedIds.set(id, isExternal)
149+
processedIds.set(cacheKey, isExternal)
145150
return isExternal
146151
}
147152
}

0 commit comments

Comments
 (0)