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.

Sign up for GitHub

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

Jump to bottom

perf(resolve): refactor tryFsResolve and tryResolveFile #12542

Merged
merged 5 commits into from Mar 23, 2023
Merged

perf(resolve): refactor tryFsResolve and tryResolveFile #12542

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e0c977
refactor(resolve): avoid options.skipPackageJson global
patak-dev Mar 22, 2023
ff9f0bc
perf: refactor tryFsResolve and tryResolveFile
patak-dev Mar 22, 2023
3e79e22
chore: use isFile()
patak-dev Mar 23, 2023
831c601
chore: use isFile()
patak-dev Mar 23, 2023
aee1e16
chore: update
patak-dev Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
310 changes: 151 additions & 159 deletions packages/vite/src/node/plugins/resolve.ts
View file
Open in desktop
Expand Up @@ -105,7 +105,6 @@ export interface InternalResolveOptions extends Required<ResolveOptions> {
asSrc?: boolean
tryIndex?: boolean
tryPrefix?: string
skipPackageJson?: boolean
preferRelative?: boolean
isRequire?: boolean
// #3040
Expand Down Expand Up @@ -487,180 +486,166 @@ function tryFsResolve(
options: InternalResolveOptions,
tryIndex = true,
targetWeb = true,
skipPackageJson = false,
): string | undefined {
const { file, postfix } = splitFileAndPostfix(fsPath)

// Dependencies like es5-ext use `#` in their paths. We don't support `#` in user
// source code so we only need to perform the check for dependencies.
const tryUnsplitted = fsPath.includes('#') && fsPath.includes('node_modules')
Copy link
Member Author

Choose a reason for hiding this comment

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

If there was a # before the query, before we were checking for tryUnsplitted branches with the full path including the query and these were all redundant checks.


let res: string | undefined

if (
tryUnsplitted &&
(res = tryResolveFile(
fsPath,
'',
options,
false,
targetWeb,
options.tryPrefix,
options.skipPackageJson,
))
) {
return res
}

if (
(res = tryResolveFile(
file,
postfix,
options,
false,
targetWeb,
options.tryPrefix,
options.skipPackageJson,
))
) {
return res
}

for (const ext of options.extensions) {
if (
tryUnsplitted &&
(res = tryResolveFile(
fsPath + ext,
'',
options,
false,
targetWeb,
options.tryPrefix,
options.skipPackageJson,
false,
))
) {
return res
}

if (
(res = tryResolveFile(
file + ext,
postfix,
let postfixIndex = fsPath.indexOf('?')
if (postfixIndex < 0) {
postfixIndex = fsPath.indexOf('#')

// Dependencies like es5-ext use `#` in their paths. We don't support `#` in user
// source code so we only need to perform the check for dependencies.
// We don't support `?` in node_modules paths, so we only need to check in this branch.
Copy link
Member Author

Choose a reason for hiding this comment

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

The function no longer used the splitFileAndPostfix util because we can avoid a redundant check if we know the result of the two indexOf.

Copy link
Contributor

Choose a reason for hiding this comment

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

if the index is used later and it is important to split path from query and hash, foo#something?bla could trip it

Copy link
Member Author

Choose a reason for hiding this comment

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

It was always the case that foo#something?bla would be split as foo#something + ?bla because a URL needs to have the ? before the #. Check how splitFileAndPostfix is working (that is being used in other places). If something is broken for node_modules path that has #, we are missing a test. Do you see a case that wouldn't work? (there definitely could be out of these two functions, but this PR shouldn't change that)

Copy link
Contributor

@dominikg dominikg Mar 23, 2023 •
edited

Choose a reason for hiding this comment

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

If it works like the previous implementation it's fine i guess (at least not a regression). But from a url perspective, a hash containing a questionmark is allowed, right? so https://example.com/path#foo?bar would have no query and hash foo?bar.
So to safely extract the path, you'd have to use the lower first index of both.

if (postfixIndex >= 0 && fsPath.includes('node_modules')) {
const res = tryCleanFsResolve(
fsPath,
options,
false,
tryIndex,
targetWeb,
options.tryPrefix,
options.skipPackageJson,
false,
))
) {
return res
skipPackageJson,
)
if (res) return res
}
}

// if `tryIndex` false, skip as we've already tested above
if (!tryIndex) return

if (
tryUnsplitted &&
(res = tryResolveFile(
fsPath,
'',
options,
tryIndex,
targetWeb,
options.tryPrefix,
options.skipPackageJson,
))
) {
return res
let file = fsPath
let postfix = ''
if (postfixIndex >= 0) {
file = fsPath.slice(0, postfixIndex)
postfix = fsPath.slice(postfixIndex)
}

if (
(res = tryResolveFile(
file,
postfix,
options,
tryIndex,
targetWeb,
options.tryPrefix,
options.skipPackageJson,
))
) {
return res
}
const res = tryCleanFsResolve(
file,
options,
tryIndex,
targetWeb,
skipPackageJson,
)
if (res) return res + postfix
}

function tryResolveFile(
function tryCleanFsResolve(
file: string,
postfix: string,
options: InternalResolveOptions,
tryIndex: boolean,
targetWeb: boolean,
tryPrefix?: string,
skipPackageJson?: boolean,
skipTsExtension?: boolean,
tryIndex = true,
targetWeb = true,
skipPackageJson = false,
): string | undefined {
let stat: fs.Stats | undefined
try {
stat = fs.statSync(file, { throwIfNoEntry: false })
} catch {
return
const { tryPrefix, extensions, preserveSymlinks } = options

const fileStat = tryStatSync(file)

// Try direct match first
if (fileStat?.isFile()) return getRealPath(file, options.preserveSymlinks)

let res: string | undefined

// If path.dirname is a valid directory, try extensions and ts resolution logic
const possibleJsToTs = options.isFromTsImporter && isPossibleTsOutput(file)
if (possibleJsToTs || extensions.length || tryPrefix) {
const dirPath = path.dirname(file)
const dirStat = tryStatSync(dirPath)
if (dirStat?.isDirectory()) {
if (possibleJsToTs) {
// try resolve .js, .mjs, .mts or .jsx import to typescript file
const tsSrcPaths = getPotentialTsSrcPaths(file)
for (const srcPath of tsSrcPaths) {
if ((res = tryResolveRealFile(srcPath, preserveSymlinks))) return res
}
}

if (
(res = tryResolveRealFileWithExtensions(
file,
extensions,
preserveSymlinks,
))
)
return res

if (tryPrefix) {
const prefixed = `${dirPath}/${options.tryPrefix}${path.basename(file)}`

if ((res = tryResolveRealFile(prefixed, preserveSymlinks))) return res

if (
(res = tryResolveRealFileWithExtensions(
prefixed,
extensions,
preserveSymlinks,
))
)
return res
}
}
}

if (stat) {
if (!stat.isDirectory()) {
return getRealPath(file, options.preserveSymlinks) + postfix
} else if (tryIndex) {
if (!skipPackageJson) {
let pkgPath = file + '/package.json'
try {
if (fs.existsSync(pkgPath)) {
if (!options.preserveSymlinks) {
pkgPath = safeRealpathSync(pkgPath)
}
// path points to a node package
const pkg = loadPackageData(pkgPath)
const resolved = resolvePackageEntry(file, pkg, targetWeb, options)
return resolved
}
} catch (e) {
if (e.code !== 'ENOENT') {
throw e
if (tryIndex && fileStat) {
// Path points to a directory, check for package.json and entry and /index file
const dirPath = file

if (!skipPackageJson) {
let pkgPath = `${dirPath}/package.json`
try {
if (fs.existsSync(pkgPath)) {
if (!options.preserveSymlinks) {
pkgPath = safeRealpathSync(pkgPath)
}
// path points to a node package
const pkg = loadPackageData(pkgPath)
return resolvePackageEntry(dirPath, pkg, targetWeb, options)
}
} catch (e) {
if (e.code !== 'ENOENT') throw e
}
const index = tryFsResolve(file + '/index', options)
if (index) return index + postfix
}
}

// try resolve .js import to typescript file
if (
!skipTsExtension &&
options.isFromTsImporter &&
isPossibleTsOutput(file)
) {
const tsSrcPaths = getPotentialTsSrcPaths(file)
for (const srcPath of tsSrcPaths) {
const res = tryResolveFile(
srcPath,
postfix,
options,
tryIndex,
targetWeb,
tryPrefix,
skipPackageJson,
true,
if (
(res = tryResolveRealFileWithExtensions(
`${dirPath}/index`,
extensions,
preserveSymlinks,
))
)
return res

if (tryPrefix) {
if (
(res = tryResolveRealFileWithExtensions(
`${dirPath}/${options.tryPrefix}index`,
extensions,
preserveSymlinks,
))
)
if (res) return res
return res
}
return
}
}

if (tryPrefix) {
const prefixed = `${path.dirname(file)}/${tryPrefix}${path.basename(file)}`
return tryResolveFile(prefixed, postfix, options, tryIndex, targetWeb)
function tryResolveRealFile(
file: string,
preserveSymlinks: boolean,
): string | undefined {
const stat = tryStatSync(file)
if (stat?.isFile()) return getRealPath(file, preserveSymlinks)
}

function tryResolveRealFileWithExtensions(
filePath: string,
extensions: string[],
preserveSymlinks: boolean,
): string | undefined {
for (const ext of extensions) {
const res = tryResolveRealFile(filePath + ext, preserveSymlinks)
if (res) return res
}
}

function tryStatSync(file: string): fs.Stats | undefined {
try {
return fs.statSync(file, { throwIfNoEntry: false })
} catch {
// Ignore errors
}
}

Expand Down Expand Up @@ -1025,22 +1010,29 @@ export function resolvePackageEntry(

for (let entry of entryPoints) {
// make sure we don't get scripts when looking for sass
let skipPackageJson = false
if (
options.mainFields[0] === 'sass' &&
!options.extensions.includes(path.extname(entry))
) {
entry = ''
options.skipPackageJson = true
Copy link
Member Author

Choose a reason for hiding this comment

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

This was modifying the options object for every entry. I think there was a bug here we never catched. I refactored skipPackageJson to pass it directly to tryFsResolve now.

}

// resolve object browser field in package.json
const { browser: browserField } = data
if (targetWeb && options.browserField && isObject(browserField)) {
entry = mapWithBrowserField(entry, browserField) || entry
skipPackageJson = true
} else {
// resolve object browser field in package.json
const { browser: browserField } = data
if (targetWeb && options.browserField && isObject(browserField)) {
entry = mapWithBrowserField(entry, browserField) || entry
}
}

const entryPointPath = path.join(dir, entry)
const resolvedEntryPoint = tryFsResolve(entryPointPath, options)
const resolvedEntryPoint = tryFsResolve(
entryPointPath,
options,
true,
true,
skipPackageJson,
)
if (resolvedEntryPoint) {
isDebug &&
debug(
Expand Down