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

fix: unoptimized package can resolve to optimized deps #11410

Closed
wants to merge 10 commits into from
12 changes: 9 additions & 3 deletions packages/vite/src/node/config.ts
Expand Up @@ -53,7 +53,7 @@ import type {
InternalResolveOptionsWithOverrideConditions,
ResolveOptions,
} from './plugins/resolve'
import { resolvePlugin, tryNodeResolve } from './plugins/resolve'
import { resolvePlugin, tryNodeResolveCore } from './plugins/resolve'
import type { LogLevel, Logger } from './logger'
import { createLogger } from './logger'
import type { DepOptimizationConfig, DepOptimizationOptions } from './optimizer'
Expand Down Expand Up @@ -997,12 +997,18 @@ async function bundleConfigFile(
}

const isIdESM = isESM || kind === 'dynamic-import'
let idFsPath = tryNodeResolve(
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling tryNodeResolve with 4 args has same effect as calling tryNodeResolveCore, but tryNodeResolve has more mental burden. So I turn it into tryNodeResolveCore call here.

let idFsPath: undefined | string
const resolveResult = tryNodeResolveCore(
id,
importer,
{ ...options, isRequire: !isIdESM },
false,
)?.id
)
if (
resolveResult.resultType === 'success' ||
resolveResult.resultType === 'fail-as-optional-peer-dep'
)
idFsPath = resolveResult.resolved
if (idFsPath && isIdESM) {
idFsPath = pathToFileURL(idFsPath).href
}
Expand Down
33 changes: 18 additions & 15 deletions packages/vite/src/node/plugins/index.ts
Expand Up @@ -6,6 +6,7 @@ import { getDepsOptimizer } from '../optimizer'
import { shouldExternalizeForSSR } from '../ssr/ssrExternal'
import { jsonPlugin } from './json'
import { resolvePlugin } from './resolve'
import type { InternalResolveOptions } from './resolve'
import { optimizedDepsBuildPlugin, optimizedDepsPlugin } from './optimizedDeps'
import { esbuildPlugin } from './esbuild'
import { importAnalysisPlugin } from './importAnalysis'
Expand Down Expand Up @@ -38,10 +39,25 @@ export async function resolvePlugins(
: { pre: [], post: [] }
const { modulePreload } = config.build

const resolveOptions: InternalResolveOptions = {
...config.resolve,
root: config.root,
isProduction: config.isProduction,
isBuild,
packageCache: config.packageCache,
ssrConfig: config.ssr,
asSrc: true,
getDepsOptimizer: (ssr: boolean) => getDepsOptimizer(config, ssr),
shouldExternalize:
isBuild && config.build.ssr && config.ssr?.format !== 'cjs'
? (id) => shouldExternalizeForSSR(id, config)
: undefined,
}

return [
isWatch ? ensureWatchPlugin() : null,
isBuild ? metadataPlugin() : null,
preAliasPlugin(config),
preAliasPlugin(config, resolveOptions),
aliasPlugin({ entries: config.resolve.alias }),
...prePlugins,
modulePreload === true ||
Expand All @@ -56,20 +72,7 @@ export async function resolvePlugins(
: optimizedDepsPlugin(config),
]
: []),
resolvePlugin({
...config.resolve,
root: config.root,
isProduction: config.isProduction,
isBuild,
packageCache: config.packageCache,
ssrConfig: config.ssr,
asSrc: true,
getDepsOptimizer: (ssr: boolean) => getDepsOptimizer(config, ssr),
shouldExternalize:
isBuild && config.build.ssr && config.ssr?.format !== 'cjs'
? (id) => shouldExternalizeForSSR(id, config)
: undefined,
}),
resolvePlugin(resolveOptions),
htmlInlineProxyPlugin(config),
cssPlugin(config),
config.esbuild !== false ? esbuildPlugin(config.esbuild) : null,
Expand Down
14 changes: 12 additions & 2 deletions packages/vite/src/node/plugins/preAlias.ts
Expand Up @@ -16,14 +16,21 @@ import {
} from '../utils'
import { getDepsOptimizer } from '../optimizer'
import { tryOptimizedResolve } from './resolve'
import type { InternalResolveOptions } from './resolve'

/**
* A plugin to avoid an aliased AND optimized dep from being aliased in src
*/
export function preAliasPlugin(config: ResolvedConfig): Plugin {
export function preAliasPlugin(
config: ResolvedConfig,
resolveOptions: InternalResolveOptions,
): Plugin {
const findPatterns = getAliasPatterns(config.resolve.alias)
const isConfiguredAsExternal = createIsConfiguredAsSsrExternal(config)
const isBuild = config.command === 'build'
const { ssrConfig } = resolveOptions
const { target: ssrTarget } = ssrConfig ?? {}

return {
name: 'vite:pre-alias',
async resolveId(id, importer, options) {
Expand All @@ -38,10 +45,13 @@ export function preAliasPlugin(config: ResolvedConfig): Plugin {
id !== '@vite/env'
) {
if (findPatterns.find((pattern) => matches(pattern, id))) {
const targetWeb = !ssr || ssrTarget === 'webworker'
const optimizedId = await tryOptimizedResolve(
depsOptimizer,
id,
importer,
resolveOptions,
targetWeb,
depsOptimizer,
)
if (optimizedId) {
return optimizedId // aliased dep already optimized
Expand Down
133 changes: 102 additions & 31 deletions packages/vite/src/node/plugins/resolve.ts
Expand Up @@ -16,6 +16,7 @@ import {
SPECIAL_QUERY_RE,
} from '../constants'
import {
assertUnreachable,
bareImportRE,
cleanUrl,
createDebugger,
Expand All @@ -35,7 +36,6 @@ import {
lookupFile,
nestedResolveFrom,
normalizePath,
resolveFrom,
slash,
} from '../utils'
import { optimizedDepInfoFromFile, optimizedDepInfoFromId } from '../optimizer'
Expand Down Expand Up @@ -309,7 +309,13 @@ export function resolvePlugin(resolveOptions: InternalResolveOptions): Plugin {
asSrc &&
depsOptimizer &&
!options.scan &&
(res = await tryOptimizedResolve(depsOptimizer, id, importer))
(res = await tryOptimizedResolve(
id,
importer,
options,
targetWeb,
depsOptimizer,
))
) {
return res
}
Expand Down Expand Up @@ -599,26 +605,27 @@ export type InternalResolveOptionsWithOverrideConditions =

export const idToPkgMap = new Map<string, PackageData>()

export function tryNodeResolve(
export type TryNodeResolveCoreResult =
| {
resultType: 'success'
resolved: string
pkg: PackageData
pkgId: string
nearestPkg: PackageData
isDeepImport: boolean
}
| { resultType: 'fail-as-optional-peer-dep'; resolved: string }
| { resultType: 'fail' }

export function tryNodeResolveCore(
id: string,
importer: string | null | undefined,
options: InternalResolveOptionsWithOverrideConditions,
targetWeb: boolean,
depsOptimizer?: DepsOptimizer,
ssr?: boolean,
externalize?: boolean,
allowLinkedExternal: boolean = true,
): PartialResolvedId | undefined {
const { root, dedupe, isBuild, preserveSymlinks, packageCache } = options
): TryNodeResolveCoreResult {
const { root, dedupe, preserveSymlinks, packageCache } = options

ssr ??= false

// split id by last '>' for nested selected packages, for example:
// 'foo > bar > baz' => 'foo > bar' & 'baz'
// 'foo' => '' & 'foo'
const lastArrowIndex = id.lastIndexOf('>')
const nestedRoot = id.substring(0, lastArrowIndex).trim()
const nestedPath = id.substring(lastArrowIndex + 1).trim()
const { nestedRoot, nestedPath } = parseNestedId(id)

const possiblePkgIds: string[] = []
for (let prevSlashIndex = -1; ; ) {
Expand Down Expand Up @@ -717,12 +724,13 @@ export function tryNodeResolve(
mainPkg.peerDependenciesMeta?.[nestedPath]?.optional
) {
return {
id: `${optionalPeerDepId}:${nestedPath}:${mainPkg.name}`,
resultType: 'fail-as-optional-peer-dep',
resolved: `${optionalPeerDepId}:${nestedPath}:${mainPkg.name}`,
}
}
}
}
return
return { resultType: 'fail' }
}

let resolveId = resolvePackageEntry
Expand Down Expand Up @@ -750,8 +758,47 @@ export function tryNodeResolve(
})
}
if (!resolved) {
return
return { resultType: 'fail' }
}

// link id to pkg for browser field mapping check
idToPkgMap.set(resolved, pkg)

return {
resultType: 'success',
resolved,
pkg,
pkgId,
nearestPkg,
isDeepImport,
}
}

export function tryNodeResolve(
id: string,
importer: string | null | undefined,
options: InternalResolveOptionsWithOverrideConditions,
targetWeb: boolean,
depsOptimizer?: DepsOptimizer,
ssr?: boolean,
externalize?: boolean,
allowLinkedExternal: boolean = true,
): PartialResolvedId | undefined {
const coreResult = tryNodeResolveCore(id, importer, options, targetWeb)
if (coreResult.resultType === 'fail') return
if (coreResult.resultType === 'fail-as-optional-peer-dep')
return {
id: coreResult.resolved,
}
if (coreResult.resultType !== 'success') {
return assertUnreachable(coreResult)
}

const { pkg, pkgId, nearestPkg, isDeepImport } = coreResult
let { resolved } = coreResult
const { isBuild } = options
ssr ??= false
const { nestedPath } = parseNestedId(id)

const processResult = (resolved: PartialResolvedId) => {
if (!externalize) {
Expand Down Expand Up @@ -784,8 +831,6 @@ export function tryNodeResolve(
return { ...resolved, id: resolvedId, external: true }
}

// link id to pkg for browser field mapping check
idToPkgMap.set(resolved, pkg)
if ((isBuild && !depsOptimizer) || externalize) {
// Resolve package side effects for build so that rollup can better
// perform tree-shaking
Expand Down Expand Up @@ -875,10 +920,24 @@ export function tryNodeResolve(
}
}

/**
* split id by last '>' for nested selected packages, for example:
* 'foo > bar > baz' => 'foo > bar' & 'baz'
* 'foo' => '' & 'foo'
*/
function parseNestedId(id: string) {
const lastArrowIndex = id.lastIndexOf('>')
const nestedRoot = id.substring(0, lastArrowIndex).trim()
const nestedPath = id.substring(lastArrowIndex + 1).trim()
return { nestedRoot, nestedPath }
}

export async function tryOptimizedResolve(
depsOptimizer: DepsOptimizer,
id: string,
importer?: string,
importer: string | null | undefined,
resolveOptions: InternalResolveOptions,
targetWeb: boolean,
depsOptimizer: DepsOptimizer,
): Promise<string | undefined> {
// TODO: we need to wait until scanning is done here as this function
// is used in the preAliasPlugin to decide if an aliased dep is optimized,
Expand All @@ -888,13 +947,15 @@ export async function tryOptimizedResolve(

const metadata = depsOptimizer.metadata

const depInfo = optimizedDepInfoFromId(metadata, id)
if (depInfo) {
return depsOptimizer.getOptimizedDepId(depInfo)
if (!importer) {
// no importer. try our best to find an optimized dep
const depInfo = optimizedDepInfoFromId(metadata, id)
if (depInfo) {
return depsOptimizer.getOptimizedDepId(depInfo)
}
return
}

if (!importer) return

// further check if id is imported by nested dependency
let resolvedSrc: string | undefined

Expand All @@ -911,8 +972,18 @@ export async function tryOptimizedResolve(
// lazily initialize resolvedSrc
if (resolvedSrc == null) {
try {
// this may throw errors if unable to resolve, e.g. aliased id
resolvedSrc = normalizePath(resolveFrom(id, path.dirname(importer)))
Copy link
Member Author

@csr632 csr632 Dec 18, 2022

Choose a reason for hiding this comment

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

The bug come from this line: resolveFrom always use package.json#main as main field, not respecting vite's main fields (module field and custom mainFields). So the resolvedSrc here is not aligned with optimizedData.src.

This PR fix it by changing this line to use tryNodeResolve which is aligned with the resolve result during deps optimization.

const resolveResult = tryNodeResolveCore(
id,
importer,
resolveOptions,
targetWeb,
)
if (resolveResult.resultType !== 'success') {
// no resolvedSrc, no need to continue
break
} else {
resolvedSrc = normalizePath(resolveResult.resolved)
}
} catch {
// this is best-effort only so swallow errors
break
Expand Down
14 changes: 10 additions & 4 deletions packages/vite/src/node/ssr/ssrModuleLoader.ts
Expand Up @@ -2,14 +2,15 @@ import path from 'node:path'
import { pathToFileURL } from 'node:url'
import type { ViteDevServer } from '../server'
import {
assertUnreachable,
dynamicImport,
isBuiltin,
unwrapId,
usingDynamicImport,
} from '../utils'
import { transformRequest } from '../server/transformRequest'
import type { InternalResolveOptionsWithOverrideConditions } from '../plugins/resolve'
import { tryNodeResolve } from '../plugins/resolve'
import { tryNodeResolveCore } from '../plugins/resolve'
import {
ssrDynamicImportKey,
ssrExportAllKey,
Expand Down Expand Up @@ -230,7 +231,7 @@ async function nodeImport(
if (id.startsWith('node:') || isBuiltin(id)) {
url = id
} else {
const resolved = tryNodeResolve(
Copy link
Member Author

@csr632 csr632 Jan 8, 2023

Choose a reason for hiding this comment

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

Calling tryNodeResolve with 4 args has same effect as calling tryNodeResolveCore, but tryNodeResolve has more mental burden. So I turn it into tryNodeResolveCore call here.

const resolveResult = tryNodeResolveCore(
id,
importer,
// Non-external modules can import ESM-only modules, but only outside
Expand All @@ -241,14 +242,19 @@ async function nodeImport(
: resolveOptions,
false,
)
if (!resolved) {
if (
resolveResult.resultType === 'success' ||
resolveResult.resultType === 'fail-as-optional-peer-dep'
) {
url = resolveResult.resolved
} else {
if (resolveResult.resultType !== 'fail') assertUnreachable(resolveResult)
const err: any = new Error(
`Cannot find module '${id}' imported from '${importer}'`,
)
err.code = 'ERR_MODULE_NOT_FOUND'
throw err
}
url = resolved.id
if (usingDynamicImport) {
url = pathToFileURL(url).toString()
}
Expand Down