-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Changes from all commits
82ad4bf
d5a2d78
7115fbf
83e328e
9e77ae1
7944bcb
c58bb0c
3b57c27
a1d50f6
62f9cb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import { | |
SPECIAL_QUERY_RE, | ||
} from '../constants' | ||
import { | ||
assertUnreachable, | ||
bareImportRE, | ||
cleanUrl, | ||
createDebugger, | ||
|
@@ -35,7 +36,6 @@ import { | |
lookupFile, | ||
nestedResolveFrom, | ||
normalizePath, | ||
resolveFrom, | ||
slash, | ||
} from '../utils' | ||
import { optimizedDepInfoFromFile, optimizedDepInfoFromId } from '../optimizer' | ||
|
@@ -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 | ||
} | ||
|
@@ -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 | ||
|
||
ssr ??= false | ||
): TryNodeResolveCoreResult { | ||
const { root, dedupe, preserveSymlinks, packageCache } = options | ||
|
||
// 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; ; ) { | ||
|
@@ -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 | ||
|
@@ -750,8 +758,45 @@ 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) { | ||
|
@@ -784,8 +829,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 | ||
|
@@ -875,10 +918,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, | ||
|
@@ -888,13 +945,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 | ||
|
||
|
@@ -911,8 +970,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))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bug come from this line: This PR fix it by changing this line to use |
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -230,7 +231,7 @@ async function nodeImport( | |
if (id.startsWith('node:') || isBuiltin(id)) { | ||
url = id | ||
} else { | ||
const resolved = tryNodeResolve( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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() | ||
} | ||
|
There was a problem hiding this comment.
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.