perf(resolve): skip for virtual files #12638
Conversation
|
|
| // This virtual file from the React plugin is hardcoded in integrations | ||
| // like Ruby & Laravel so adding this instead of migrating both React | ||
| // plugins to use /virtual: | ||
| id === '/@react-refresh' |
There was a problem hiding this comment.
I'm not sure about hardcoding this, don't we already resolve this id in a pre plugin?
That means it wouldn't hit here if it resolved there first.
There was a problem hiding this comment.
The SWC plugin is not pre. But maybe I can split the plugin in two so that the client is pre and the rest is not pre.
I think I will do the same because it's not easy to add pre-process of other plugin for example: https://github.com/vitejs/vite-plugin-react/pull/122/files?w=1#diff-885677e87d0c22a62efc8688d62685a305a3eb34469ddf0d2857da84cf417630R7
| // When injected directly in html/client code | ||
| id.startsWith('/virtual:') || |
There was a problem hiding this comment.
I'm not sure you can inject this directly in html/client and work 🤔 Do you have an example of this?
There was a problem hiding this comment.
unoCSS for example: https://github.com/unocss/unocss/blob/main/packages/vite/src/devtool.ts#L141
(here it's not using the "/virtual:" prefix but it could)
There was a problem hiding this comment.
That means it could be anything though, and we shouldn't have a special preference for /virtual:?
There was a problem hiding this comment.
I think we should provide and encourage using a prefix that both:
- doesn't trigger fs calls if the plugin is not "pre"
- is a valid url and doesn't need to be pre-process by Vite
Without this line, the line you need to use /@id/__x00__@unocss/devtools which is not great compare to /virtual:@unocss/devtools
There was a problem hiding this comment.
The use of \0 helps because the rollup ecosystem expects that when handling virtual modules (there are checks in rollup plugins when dealing with source maps for example). Uno should use \0 in the resolved id here.
I think that converting \0 to /@id/__x00__ is making folks shy away from using the proper virtual marker. We could change our scheme in Vite 5 (or before?) to something that looks better. For example /@virtual/ or /@id/virtual:. @antfu what do you think?
There was a problem hiding this comment.
I'm fine adding this condition. I still think we should keep suggesting the use of \0 (and to me changing our encoding could help there, I may do a PR in Vite 5 about this)
There was a problem hiding this comment.
I agree to remove it until we agree on a pattern.
But when you look at profiling, import parsing & transformation starts to show up and I think it would be good to move to import patterns that are more ESM friendly
There was a problem hiding this comment.
What do you mean by ESM friendly? Having to encode \0 for the browser shouldnt have a noticeable perf hit, no?
There was a problem hiding this comment.
The perf hit is in parsing/rewriting every bit of JS we're sending to the browser.
And rewriting all import implies a lot of string manipulation where JS is not really fast.
I can't find a specific entry for that in a profile right now, but this is one of the main bottleneck on rds.
But when you look at a profile, you see a lot of small things that are related to import transformation (new URL which is really slow in node, regex, ensureVersion, isOptimisedDep, ...)
If we rethink the patterns, we should go for things that does't need serialization and can directly go to as fs.read call when hitting back the server.
There was a problem hiding this comment.
I don't think we can avoid resolving the URLs though. We at least need to check them once. My point is that \0 vs /virtual: doesn't affect this in particular. We still need to improve things while resolving, even after all we did in the past weeks, there seems to be a lot more to gain from making vite:resolve leaner. new URL is a good example, we should check if we can replace that by something faster.
About this PR, let's keep the new /virtual: condition, and remove the `'/@react-refresh' and we can merge it?
Integrations links: Ruby & Laravel