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): avoid tryFsResolve for /@fs/ paths #12450

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

patak-dev
Copy link
Member

Description

/@fs/ should already be resolved. I think we shouldn't need a full tryFsResolve (that checks for extensions and package entries). And we don't even need to check if the file exists.

Doing a PR to test CI.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Mar 16, 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

vite-ecosystem-ci bot commented Mar 16, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
previewjs ✅ success
qwik ❌ failure
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

@patak-dev patak-dev changed the title refactor: avoid tryFsResolve for /@fs/ paths perf(resolve): avoid tryFsResolve for /@fs/ paths Mar 16, 2023
@patak-dev patak-dev added the performance Performance related enhancement label Mar 16, 2023
@patak-dev patak-dev requested a review from bluwy March 16, 2023 21:28
@patak-dev
Copy link
Member Author

It seems that ecosystem CI is happy too. @antfu double-checking that as a plugin author, you don't expect to be able to use /@fs/... with an unresolved path.

Liknox

This comment was marked as spam.

@patak-dev
Copy link
Member Author

@Liknox if you spam the repo again, you'll be banned from the org (context: three other PRs got approvals or a generic comment). You're welcome to do reviews that are focused on the reason to move or not forward with a PR

@bluwy
Copy link
Member

bluwy commented Mar 17, 2023

Astro used to rely on a hack with /@fs, where when it imports /@fs/.../Component.jsx, it wants to resolve to /@fs/.../Component.tsx, but Vite doesn't resolve that way, so Astro imported /@fs/.../Component instead which worked. This is resolved in Astro side now though, but putting out this usecase for reference.

I think vite-node / vitest relies on manually crafting /@fs too so would be good to check with @antfu and @sheremet-va. But since mucking around /@fs is internal API, this should be good even so.

@sheremet-va
Copy link
Member

I think vite-node / vitest relies on manually crafting /@fs too so would be good to check with @antfu and @sheremet-va. But since mucking around /@fs is internal API, this should be good even so.

Vitest doesn't rely on manually crafting /@fs, we always use pluginContainer.resolve. Vite-Node actually expects /@fs/ to always point to the actual file on the file system, and doesn't check its existence.

@antfu
Copy link
Member

antfu commented Mar 17, 2023

I am not very aware of the context here. But I think if it passes for Vitest it's probably also ok with other usages.

@patak-dev
Copy link
Member Author

Thanks for the PR to Astro and the context @bluwy. cc @danielroe @benmccann @brillout @Akryum, just in case you also relied on hacks using unresolved /@fs/ ids.

But that's a good point @antfu. Maybe we could include this one on Vite 4.3. I'll add it to the team board so we can check with Evan too because looking at the history, resolving after unwrapping /@fs/ paths was intentional at first. I think there may have been cases where the equivalent to the import analysis plugin didn't fully resolve the files and just prefixed certain import paths

@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Mar 17, 2023
@brillout
Copy link
Contributor

👍 Sounds good to me.

@patak-dev
Copy link
Member Author

We discussed this PR in today's team meeting and decided to move forward with it. Merging so we can test during the beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants