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.

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

refactor: simplify resolveSSRExternal #5210

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 6 additions & 8 deletions packages/vite/src/node/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -371,14 +371,12 @@ export async function createServer(
},
transformIndexHtml: null!, // to be immediately set
ssrLoadModule(url) {
if (!server._ssrExternals) {
server._ssrExternals = resolveSSRExternal(
config,
server._optimizeDepsMetadata
? Object.keys(server._optimizeDepsMetadata.optimized)
: []
)
}
server._ssrExternals ||= resolveSSRExternal(
config,
server._optimizeDepsMetadata
? Object.keys(server._optimizeDepsMetadata.optimized)
: []
)
return ssrLoadModule(url, server)
},
ssrFixStacktrace(e) {
Expand Down
130 changes: 63 additions & 67 deletions packages/vite/src/node/ssr/ssrExternal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ import path from 'path'
import { tryNodeResolve, InternalResolveOptions } from '../plugins/resolve'
import {
createDebugger,
isDefined,
lookupFile,
normalizePath,
resolveFrom,
unique
resolveFrom
} from '../utils'
import { ResolvedConfig } from '..'
import { createFilter } from '@rollup/pluginutils'
Expand All @@ -27,22 +25,50 @@ export function resolveSSRExternal(
ssrExternals: Set<string> = new Set(),
seen: Set<string> = new Set()
): string[] {
if (config.ssr?.noExternal === true) {
const ssrConfig = config.ssr
if (ssrConfig?.noExternal === true) {
return []
}
ssrConfig?.external?.forEach((id) => {
ssrExternals.add(id)
seen.add(id)
})

collectExternals(config.root, ssrExternals, seen)

const { root } = config
for (const dep of knownImports) {
// assume external if not yet seen
if (!seen.has(dep)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, the project root and any linked packages have had their dependencies checked, so we can safely mark any knownImports not yet seen as external. They are guaranteed to be dependencies of packages in node_modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should your comment here perhaps be a code comment?

ssrExternals.add(dep)
}
}

ssrExternals.delete('vite')

let externals = [...ssrExternals]
if (ssrConfig?.noExternal) {
externals = externals.filter(
createFilter(undefined, ssrConfig.noExternal, { resolve: false })
)
}
return externals
}

function collectExternals(
root: string,
ssrExternals: Set<string>,
seen: Set<string>
) {
const pkgContent = lookupFile(root, ['package.json'])
if (!pkgContent) {
return []
return
}

const pkg = JSON.parse(pkgContent)
const importedDeps = knownImports.map(getNpmPackageName).filter(isDefined)
const deps = unique([
...importedDeps,
...Object.keys(pkg.devDependencies || {}),
...Object.keys(pkg.dependencies || {})
])
const deps = {
...pkg.devDependencies,
...pkg.dependencies
}

const resolveOptions: InternalResolveOptions = {
root,
Expand All @@ -52,10 +78,8 @@ export function resolveSSRExternal(

const depsToTrace = new Set<string>()

for (const id of deps) {
if (seen.has(id)) {
continue
}
for (const id in deps) {
if (seen.has(id)) continue
seen.add(id)

let entry: string | undefined
Expand All @@ -73,64 +97,46 @@ export function resolveSSRExternal(
// which returns with '/', require.resolve returns with '\\'
requireEntry = normalizePath(require.resolve(id, { paths: [root] }))
} catch (e) {
try {
// no main entry, but deep imports may be allowed
const pkgPath = resolveFrom(`${id}/package.json`, root)
if (pkgPath.includes('node_modules')) {
ssrExternals.add(id)
} else {
depsToTrace.add(path.dirname(pkgPath))
}
continue
} catch {}

// resolve failed, assume include
debug(`Failed to resolve entries for package "${id}"\n`, e)
continue
}
// no esm entry but has require entry
if (!entry) {
// no esm entry but has require entry (is this even possible?)
ssrExternals.add(id)
continue
}
if (!entry.includes('node_modules')) {
// entry is not a node dep, possibly linked - don't externalize
// instead, trace its dependencies.
depsToTrace.add(id)
continue
// trace the dependencies of linked packages
else if (!entry.includes('node_modules')) {
const pkgPath = resolveFrom(`${id}/package.json`, root)
depsToTrace.add(path.dirname(pkgPath))
}
if (entry !== requireEntry) {
// has separate esm/require entry, assume require entry is cjs
// has separate esm/require entry, assume require entry is cjs
else if (entry !== requireEntry) {
ssrExternals.add(id)
} else {
// node resolve and esm resolve resolves to the same file.
if (!/\.m?js$/.test(entry)) {
// entry is not js, cannot externalize
continue
}
// check if the entry is cjs
}
// externalize js entries with commonjs
Copy link
Collaborator

Choose a reason for hiding this comment

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

there changes are conflicting with the changes in https://github.com/vitejs/vite/pull/5197/files#diff-a6d6596967a09b96fa100c4b77480490b5ef95df88987ecbfa00ba1c868e4f53R97

rebased version is here: #5544

if you'd prefer to update this PR, I'm happy to close mine

else if (/\.m?js$/.test(entry)) {
const content = fs.readFileSync(entry, 'utf-8')
if (/\bmodule\.exports\b|\bexports[.\[]|\brequire\s*\(/.test(content)) {
ssrExternals.add(id)
}
}
}

for (const id of depsToTrace) {
const depRoot = path.dirname(
resolveFrom(`${id}/package.json`, root, !!config.resolve.preserveSymlinks)
)
resolveSSRExternal(
{
...config,
root: depRoot
},
knownImports,
ssrExternals,
seen
)
}

if (config.ssr?.external) {
config.ssr.external.forEach((id) => ssrExternals.add(id))
}
let externals = [...ssrExternals]
if (config.ssr?.noExternal) {
const filter = createFilter(undefined, config.ssr.noExternal, {
resolve: false
})
externals = externals.filter((id) => filter(id))
for (const depRoot of depsToTrace) {
collectExternals(depRoot, ssrExternals, seen)
}
return externals.filter((id) => id !== 'vite')
}

export function shouldExternalizeForSSR(
Expand All @@ -149,13 +155,3 @@ export function shouldExternalizeForSSR(
})
return should
}

function getNpmPackageName(importPath: string): string | null {
const parts = importPath.split('/')
if (parts[0].startsWith('@')) {
if (!parts[1]) return null
return `${parts[0]}/${parts[1]}`
} else {
return parts[0]
}
}