Skip to content

perf: avoid computing paths on each request #15318

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

Merged
merged 2 commits into from
Dec 12, 2023
Merged

perf: avoid computing paths on each request #15318

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e265297
perf: avoid computing paths on each request
patak-dev Dec 11, 2023
24a9045
chore: update
patak-dev Dec 12, 2023
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
78 changes: 41 additions & 37 deletions packages/vite/src/node/server/middlewares/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ export function transformMiddleware(
server: ViteDevServer,
): Connect.NextHandleFunction {
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`

// check if public dir is inside root dir
const publicDir = normalizePath(server.config.publicDir)
const rootDir = normalizePath(server.config.root)
Copy link
Member

@sapphi-red sapphi-red Dec 12, 2023

Choose a reason for hiding this comment

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

vite/packages/vite/src/node/config.ts

Lines 490 to 492 in ea6a7a6

const resolvedRoot = normalizePath(
config.root ? path.resolve(config.root) : process.cwd(),
)

vite/packages/vite/src/node/config.ts

Line 738 in ea6a7a6

root: resolvedRoot,

config.root is already normalized, so we don't need to normalize it here.
I think we can normalize the publicDir when we resolve the config as well.

vite/packages/vite/src/node/config.ts

Lines 650 to 656 in ea6a7a6

const resolvedPublicDir =
publicDir !== false && publicDir !== ''
? path.resolve(
resolvedRoot,
typeof publicDir === 'string' ? publicDir : 'public',
)
: ''

Copy link
Member Author

@patak-dev patak-dev Dec 12, 2023

Choose a reason for hiding this comment

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

Done with config.root. About publicDir, we got hit by it not being normalized here #15317, so I think it is a good idea. But I was thinking we should do this in the next minor or major because some plugins may expect it to not be normalized. Hopefully there aren't many instances like that but better to play safe.

I think that all the directory paths in the resolved config should be normalized and without a trailing slash: root, publicDir, outDir. We should check what we are doing with base. There are a lot of places were we are doing a path.posix.join or similar when we could be directly concatenating the strings if we trust the resolved config.

const publicDirInRootDir = publicDir.startsWith(withTrailingSlash(rootDir))
const publicPath = `${publicDir.slice(rootDir.length)}/`

return async function viteTransformMiddleware(req, res, next) {
if (req.method !== 'GET' || knownIgnoreList.has(req.url!)) {
return next()
Expand Down Expand Up @@ -123,43 +130,8 @@ export function transformMiddleware(
}
}

// check if public dir is inside root dir
const publicDir = normalizePath(server.config.publicDir)
const rootDir = normalizePath(server.config.root)
if (publicDir.startsWith(withTrailingSlash(rootDir))) {
const publicPath = `${publicDir.slice(rootDir.length)}/`
// warn explicit public paths
if (url.startsWith(withTrailingSlash(publicPath))) {
let warning: string

if (isImportRequest(url)) {
const rawUrl = removeImportQuery(url)
if (urlRE.test(url)) {
warning =
`Assets in the public directory are served at the root path.\n` +
`Instead of ${colors.cyan(rawUrl)}, use ${colors.cyan(
rawUrl.replace(publicPath, '/'),
)}.`
} else {
warning =
'Assets in public directory cannot be imported from JavaScript.\n' +
`If you intend to import that asset, put the file in the src directory, and use ${colors.cyan(
rawUrl.replace(publicPath, '/src/'),
)} instead of ${colors.cyan(rawUrl)}.\n` +
`If you intend to use the URL of that asset, use ${colors.cyan(
injectQuery(rawUrl.replace(publicPath, '/'), 'url'),
)}.`
}
} else {
warning =
`Files in the public directory are served at the root path.\n` +
`Instead of ${colors.cyan(url)}, use ${colors.cyan(
url.replace(publicPath, '/'),
)}.`
}

server.config.logger.warn(colors.yellow(warning))
}
if (publicDirInRootDir && url.startsWith(publicPath)) {
warnAboutExplicitPublicPathInUrl(url)
}

if (
Expand Down Expand Up @@ -265,4 +237,36 @@ export function transformMiddleware(

next()
}

function warnAboutExplicitPublicPathInUrl(url: string) {
let warning: string

if (isImportRequest(url)) {
const rawUrl = removeImportQuery(url)
if (urlRE.test(url)) {
warning =
`Assets in the public directory are served at the root path.\n` +
`Instead of ${colors.cyan(rawUrl)}, use ${colors.cyan(
rawUrl.replace(publicPath, '/'),
)}.`
} else {
warning =
'Assets in public directory cannot be imported from JavaScript.\n' +
`If you intend to import that asset, put the file in the src directory, and use ${colors.cyan(
rawUrl.replace(publicPath, '/src/'),
)} instead of ${colors.cyan(rawUrl)}.\n` +
`If you intend to use the URL of that asset, use ${colors.cyan(
injectQuery(rawUrl.replace(publicPath, '/'), 'url'),
)}.`
}
} else {
warning =
`Files in the public directory are served at the root path.\n` +
`Instead of ${colors.cyan(url)}, use ${colors.cyan(
url.replace(publicPath, '/'),
)}.`
}

server.config.logger.warn(colors.yellow(warning))
}
}