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

perf(resolve): reduce vite client path checks #12471

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

patak-dev
Copy link
Member

Description

@vite/client is an alias replaced by the resolved path to the vite client /root/.../client.mjs. This id reaches the resolve plugin first and it is then tested against root here. So tryFsResolve is called with /root/root/.../client.mjs, resulting in failed checks (like /root/root/.../client.mjs.ts, and all the default extensions).

This PR modifies the alias to use /@fs/root/.../client.mjs, so the resolve plugin will directly try this path and resolve it.

If #12450 is merged, then there will not even be a fs check for this path.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Mar 17, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added the performance Performance related enhancement label Mar 17, 2023
@patak-dev patak-dev added this to the 4.3 milestone Mar 17, 2023
@patak-dev patak-dev changed the title perf: reduce vite client path checks perf(resolve): reduce vite client path checks Mar 17, 2023
@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 17, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@bluwy
Copy link
Member

bluwy commented Mar 18, 2023

Given resolve.alias is usually configured to map to absolute paths when the user configures it, can we optimize for all case of that? We probably need a cheap heuristic to tell whether a /... path is root-relative or absolute, I think that will help projects that uses aliases a lot.

(Currently trying some things out locally)

@patak-dev
Copy link
Member Author

The problem is what happens if you alias to {root}/file.js and your project has a /{root}/{root}/file.js. How resolve works now is that an absolute URL, starting from the root should win here. It makes sense to avoid clashes with the file system if there are /{root}/a.js and /a.js in the fs for example. Maybe if the absolute path starts with /{root} we could do an exception, but I'm afraid of the edge cases it may bring.

The other issue is that aliases may not be resolved, so we could no longer do #12450 (maybe we need a new ?resolved concept to bring that back, but sheremet said that Vitest also expects /@fs/ to be resolved).

@bluwy
Copy link
Member

bluwy commented Mar 18, 2023

The problem is what happens if you alias to {root}/file.js and your project has a /{root}/{root}/file.js

Yeah I was thinking we can do a one-time pass to verify if /{root}/{root} exist, and perform that root-relative resolve if it exists. In most projects I don't think it does so we can optimize that.

The other issue is that aliases may not be resolved, so we could no longer do #12450 (maybe we need a new ?resolved concept to bring that back, but sheremet said that Vitest also expects /@fs/ to be resolved).

I don't think it would affect #12450 since the idea doesn't interfere with @fs 🤔 Import analysis would resolve the absolute path to the actual file before prepending the @fs here

bluwy
bluwy previously approved these changes Mar 18, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Through some test, optimizing alias that maps to absolute paths is doable, but makes the code a bit complex. They are usually resolved under 1ms too, so I don't think it's worth it for now. Since this Vite client path is something we control, I think we can try this 👍

Comment on lines 476 to 477
{ find: /^\/?@vite\/env/, replacement: FS_PREFIX + ENV_ENTRY },
{ find: /^\/?@vite\/client/, replacement: FS_PREFIX + CLIENT_ENTRY },
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should also clean up and do path.posix.join instead. It's done at ,

rtn = path.posix.join(FS_PREFIX, id)

url = path.posix.join(FS_PREFIX, resolved.id)

but not at
const url = `${FS_PREFIX}${file}`

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to path.posix.join in vitejs/vite@656e5d2 (#12471)

Should we add isFsPathId and fsPathToId in another PR and use them everywhere?

export function isFsPathId(id: string): boolean {
  return id.startsWith(FS_PREFIX)
}

export function fsPathToId(fsPath: string): string {
  return path.posix.join(FS_PREFIX, normalizePath(fsPath))
}

export function fsPathFromId(fsPath: string): string {

Copy link
Member

Choose a reason for hiding this comment

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

Forgot to reply 😅 I don't really have a hard opinion if we want to abstract it, but usually I lean on not abstracting things until it becomes a problem.


Also maybe we can go ahead with this PR too now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think maybe we wait for this one a bit more. If #12450 is merged, then we will win a bit, but if not, I think we won't be winning much.

@patak-dev
Copy link
Member Author

Let's try this one then now that #12450 has been merged

@patak-dev patak-dev enabled auto-merge (squash) April 6, 2023 20:18
@patak-dev patak-dev disabled auto-merge April 6, 2023 20:47
@patak-dev patak-dev merged commit c49af23 into main Apr 6, 2023
@patak-dev patak-dev deleted the perf/reduce-vite-client-path-checks branch April 6, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants