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: temporary hack to avoid fs checks for /@react-refresh #15299

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Dec 10, 2023

Description

Edit: after internal discussions, the PR is only doing the workaround for /@react-refresh. We'll remove this check once we have a proper solution to move the vite-plugin-react to use the \0 convention or decide to properly formalize a new convention. @bluwy was proposing a new meta flag returned from resolve id for example. I would prefer that conventions keep being reflected on the path itself. Maybe we should expose a function isVirtualModule(id) so we can at one point have an easier time in case a change in conventions is needed.

In the profile for windows shared by @sapphi-red (recorded against #15279), the internal normalizeUrl in the import analysis plugin is now taking more time than resolveId. Most of the time is spent in fs.existsSync.

image

This is 2% of the total work now.

The cached fs tree isn't kicking here because we are checking if an out-of-root absolute path exists in the fs. But the real problem is that we are calling fs.existsSync('/@react-refresh') over and over in this benchmark.

vite-plugin-react-swc is using /@react-refresh both for authoring, and as the resolved id. See https://github.com/vitejs/vite-plugin-react-swc/blob/37f002cc9ecdf7e616004ad61ad9eeb3e3b6e873/src/index.ts#L73C8-L73C8. It isn't prefixing the id with the virtual file convention of \0.

We could start prefixing the resolved virtual module with \0. @ArnaudBarre, I think we should try to add that in the next major. I'm a bit afraid though in this case because there may be integrations that expect the resolved id to be /@react-refresh. It seems the ecosystem has this id harcoded everywhere. I think there are some plugins that could break here: https://github.com/search?q=%2F%40react-refresh&type=code

The other problem is that /@plugin-name/xxx is a convention that the ecosystem has widely used for resolved ids. This was started by us, with /@vite/client, /@fs/..., /@id/..., and /@react-refresh. And I saw /@vite-plugin-pwa, /@vite-plugin-pages/..., among others.

Given that we already don't support URLs that are absolute paths starting with /@vite, /@fs, /@id at the root of the filesystem. This PR generalizes that to URLs starting with /@. It only touches the check that is causing issues, which is done after resolving so plugins can't avoid it (there is even a comment in the react plugin about using order pre to avoid fs calls).

This is only for the root of the filesystem, and not the root of the project. We don't support /@vite, /@fs, /@id either at the root of the project though. I think we could in the future (next minor or major), formalize that /@... is a valid virtual module convention and also avoid checks at the root of the project. For now, I only added this check so we stop doing these expensive fs exists calls.

A detail, in my M1, it seems it these checks aren't that expensive. We could also avoid the check only for Windows, where it makes even less sense to be checking for these paths, but I think it is better to do it for all platforms.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Dec 10, 2023

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

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 877b184: Open

suite result latest scheduled
analogjs success success
astro success success
histoire success success
ladle success success
laravel failure failure
marko success success
nuxt failure failure
nx success success
previewjs success success
qwik success success
rakkas failure success
sveltekit success success
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success success

@patak-dev patak-dev closed this Dec 10, 2023
@patak-dev patak-dev reopened this Dec 10, 2023
@patak-dev
Copy link
Member Author

/ecosystem-ci run rakkas

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 877b184: Open

suite result latest scheduled
rakkas success success

ArnaudBarre
ArnaudBarre previously approved these changes Dec 10, 2023
Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

In an ideal world I would like the rollup plugin API with the namespace notion of esbuild.
This sounds reasonable for me, I think we should let bundler have some reserved chars before meta framework takes them all 😁

@patak-dev patak-dev changed the title perf: avoid fs check for absolute path starting with /@ perf: temporal workaround to avoid fs checks for /@react-refresh Dec 12, 2023
@patak-dev patak-dev added the performance Performance related enhancement label Dec 12, 2023
@patak-dev patak-dev changed the title perf: temporal workaround to avoid fs checks for /@react-refresh perf: temporary hack to avoid fs checks for /@react-refresh Dec 12, 2023
@patak-dev patak-dev merged commit b1d6211 into main Dec 12, 2023
15 checks passed
@patak-dev patak-dev deleted the perf/slash-at-virtual-module-convention branch December 12, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants