Skip to content

Commit

Permalink
fix(ssr): fix crash when a pnpm/Yarn workspace depends on a CJS packa…
Browse files Browse the repository at this point in the history
…ge (#9763)
  • Loading branch information
rtsao authored Jun 13, 2023
1 parent 6a87c65 commit 9e1086b
Show file tree
Hide file tree
Showing 11 changed files with 57 additions and 28 deletions.
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/importAnalysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
) {
return
}
} else if (shouldExternalizeForSSR(specifier, config)) {
} else if (shouldExternalizeForSSR(specifier, importer, config)) {
return
}
if (isBuiltin(specifier)) {
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export async function resolvePlugins(
getDepsOptimizer: (ssr: boolean) => getDepsOptimizer(config, ssr),
shouldExternalize:
isBuild && config.build.ssr && config.ssr?.format !== 'cjs'
? (id) => shouldExternalizeForSSR(id, config)
? (id, importer) => shouldExternalizeForSSR(id, importer, config)
: undefined,
}),
htmlInlineProxyPlugin(config),
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/preAlias.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export function preAliasPlugin(config: ResolvedConfig): Plugin {
(isInNodeModules(resolvedId) ||
optimizeDeps.include?.includes(id)) &&
isOptimizable(resolvedId, optimizeDeps) &&
!(isBuild && ssr && isConfiguredAsExternal(id)) &&
!(isBuild && ssr && isConfiguredAsExternal(id, importer)) &&
(!ssr || optimizeAliasReplacementForSSR(resolvedId, optimizeDeps))
) {
// aliased dep has not yet been optimized
Expand Down
4 changes: 2 additions & 2 deletions packages/vite/src/node/plugins/resolve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export interface InternalResolveOptions extends Required<ResolveOptions> {
ssrOptimizeCheck?: boolean
// Resolve using esbuild deps optimization
getDepsOptimizer?: (ssr: boolean) => DepsOptimizer | undefined
shouldExternalize?: (id: string) => boolean | undefined
shouldExternalize?: (id: string, importer?: string) => boolean | undefined

/**
* Set by createResolver, we only care about the resolved id. moduleSideEffects
Expand Down Expand Up @@ -329,7 +329,7 @@ export function resolvePlugin(resolveOptions: InternalResolveOptions): Plugin {

// bare package imports, perform node resolve
if (bareImportRE.test(id)) {
const external = options.shouldExternalize?.(id)
const external = options.shouldExternalize?.(id, importer)
if (
!external &&
asSrc &&
Expand Down
26 changes: 15 additions & 11 deletions packages/vite/src/node/ssr/ssrExternal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,24 +92,25 @@ const _require = createRequire(import.meta.url)

const isSsrExternalCache = new WeakMap<
ResolvedConfig,
(id: string) => boolean | undefined
(id: string, importer?: string) => boolean | undefined
>()

export function shouldExternalizeForSSR(
id: string,
importer: string | undefined,
config: ResolvedConfig,
): boolean | undefined {
let isSsrExternal = isSsrExternalCache.get(config)
if (!isSsrExternal) {
isSsrExternal = createIsSsrExternal(config)
isSsrExternalCache.set(config, isSsrExternal)
}
return isSsrExternal(id)
return isSsrExternal(id, importer)
}

export function createIsConfiguredAsSsrExternal(
config: ResolvedConfig,
): (id: string) => boolean {
): (id: string, importer?: string) => boolean {
const { ssr, root } = config
const noExternal = ssr?.noExternal
const noExternalFilter =
Expand All @@ -126,6 +127,7 @@ export function createIsConfiguredAsSsrExternal(

const isExternalizable = (
id: string,
importer?: string,
configuredAsExternal?: boolean,
): boolean => {
if (!bareImportRE.test(id) || id.includes('\0')) {
Expand All @@ -134,7 +136,9 @@ export function createIsConfiguredAsSsrExternal(
try {
return !!tryNodeResolve(
id,
undefined,
// Skip passing importer in build to avoid externalizing non-hoisted dependencies
// unresolveable from root (which would be unresolvable from output bundles also)
config.command === 'build' ? undefined : importer,
resolveOptions,
ssr?.target === 'webworker',
undefined,
Expand All @@ -157,7 +161,7 @@ export function createIsConfiguredAsSsrExternal(

// Returns true if it is configured as external, false if it is filtered
// by noExternal and undefined if it isn't affected by the explicit config
return (id: string) => {
return (id: string, importer?: string) => {
const { ssr } = config
if (ssr) {
if (
Expand All @@ -169,14 +173,14 @@ export function createIsConfiguredAsSsrExternal(
}
const pkgName = getNpmPackageName(id)
if (!pkgName) {
return isExternalizable(id)
return isExternalizable(id, importer)
}
if (
// A package name in ssr.external externalizes every
// externalizable package entry
ssr.external?.includes(pkgName)
) {
return isExternalizable(id, true)
return isExternalizable(id, importer, true)
}
if (typeof noExternal === 'boolean') {
return !noExternal
Expand All @@ -185,24 +189,24 @@ export function createIsConfiguredAsSsrExternal(
return false
}
}
return isExternalizable(id)
return isExternalizable(id, importer)
}
}

function createIsSsrExternal(
config: ResolvedConfig,
): (id: string) => boolean | undefined {
): (id: string, importer?: string) => boolean | undefined {
const processedIds = new Map<string, boolean | undefined>()

const isConfiguredAsExternal = createIsConfiguredAsSsrExternal(config)

return (id: string) => {
return (id: string, importer?: string) => {
if (processedIds.has(id)) {
return processedIds.get(id)
}
let external = false
if (id[0] !== '.' && !path.isAbsolute(id)) {
external = isBuiltin(id) || isConfiguredAsExternal(id)
external = isBuiltin(id) || isConfiguredAsExternal(id, importer)
}
processedIds.set(id, external)
return external
Expand Down
12 changes: 12 additions & 0 deletions playground/ssr-deps/nested-external-cjs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Module with state, to check that it is properly externalized and
// not bundled in the optimized deps
let msg

module.exports = {
setMessage(externalMsg) {
msg = externalMsg
},
getMessage() {
return msg
},
}
7 changes: 7 additions & 0 deletions playground/ssr-deps/nested-external-cjs/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "nested-external-cjs",
"private": true,
"version": "0.0.0",
"main": "index.js",
"type": "commonjs"
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { setMessage } from 'nested-external'
import external from 'nested-external-cjs'

setMessage('Hello World!')
external.setMessage('Hello World!')
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"type": "module",
"main": "index.js",
"dependencies": {
"nested-external": "file:../nested-external"
"nested-external": "file:../nested-external",
"nested-external-cjs": "file:../nested-external-cjs"
}
}
2 changes: 1 addition & 1 deletion playground/ssr-deps/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"@vitejs/test-no-external-cjs": "file:./no-external-cjs",
"@vitejs/test-import-builtin-cjs": "file:./import-builtin-cjs",
"@vitejs/test-no-external-css": "file:./no-external-css",
"@vitejs/test-non-optimized-with-nested-external": "file:./non-optimized-with-nested-external",
"@vitejs/test-non-optimized-with-nested-external": "workspace:*",
"@vitejs/test-optimized-with-nested-external": "file:./optimized-with-nested-external",
"@vitejs/test-optimized-cjs-with-nested-external": "file:./optimized-with-nested-external",
"@vitejs/test-external-using-external-entry": "file:./external-using-external-entry",
Expand Down
23 changes: 13 additions & 10 deletions pnpm-lock.yaml

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

0 comments on commit 9e1086b

Please sign in to comment.