-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: legacy avoid insert the entry module css #9761
Conversation
// legacy build and inline css | ||
|
||
// the legacy build should avoid inserting entry CSS modules here, they | ||
// will be collected into `chunk.viteMetadata.importedCss` and injected | ||
// later by the `'vite:build-html'` plugin into the `index.html` | ||
if (chunk.isEntry) { | ||
return null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't quite caught up with the recent legacy changes, but are we now moving legacy-specific code into core? (just curious 😬)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch has always been in the css
plugin. It would be great to move it plugin-legacy but there are some details where we are cheating from the inception of the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe no. The previous PR #9507 fix the legacy build css (this if branch) no replace the asset flag(__VITE_ASSET) because the css-post
later than asset
. This PR optimized legacy build css generate logic, and this if branch will trigger by legcy build. 😊
@patak-dev @sapphi-red, can you please explain me how after this logic, the JS navigation between entry pages, say in Nuxt.js, should work under Vite on legacy mode? This PR should have canceled the ability to emulate in JS the navigation between one entry page to the other. In modern, the navigation performs preloading that loads the CSS, but in legacy, no preloading will happen. Indeed, when I worked on SvelteKit legacy support as I mentioned in #9902, the CSS where not inlined because of this PR that were merged. This was the main motivation for me to work on PR #9920, that clearly (relatively simply) solves all the compatibility issues, by preloading CSS both on modern and legacy. |
IIUC this PR is fixing #2062 (comment) by removing the inlined one. |
So am I right in my observation that this PR breaks some good behavior, and now the CSS loading on navigation for entry files is breaking on legacy, not only on SvelteKit(on legacy, by my external proposal on SvelteKit)? |
do you mean some preload link |
No. What I mean is this: If you serve static SSR, and the navigation between pages isn't handled by JS(such in Next.js, Nuxt and SvelteKit), nothing is wrong in this because the CSS files were already loaded on initial SSR in the For example, if you're using Nuxt and you on page "/"(homepage), and the user navigates to the "/about" page, the page wouldn't be loaded again by SSR, rather the navigation will be dynamic by JS code. |
It mean the navigation by client js don't deps on SSR again. But I don't know why the I guess the IIUC, your PR #9920 maybe can resolve this, make the css as chunk and dynamic load is also can reduce the legacy mode size. 👍 |
My this comment (#9761 (comment)) was talking about
If |
yes, It may be two case to replay.
|
But now what will happen on legacy mode when the user first load the "/about" page(which is serve by SSR with the CSS content in This is a consensus in all the SSR/SSG methodology, to treat every page (by default) as entry. |
If it works like that, I guess it's a server-side routing. Or it has a custom handing that isn't handled by Vite. |
Are you suggesting that Vite shouldn't care at all (even not even via some configuration varaible?) a client side routing between two entry points? It just seems a crucial part of Vite preloading. Can someone test the scenario I described earlier in Vue&Nuxt when the mode is legacy? It might have the same critical bug there I were facing with SvelteKit legacy. I just don't Vue enough to do so by myself. If we don't want to merge #9920 (hope it will be merged finally though), here is an alternative solution that reduces the double loading of entry points CSS(i.e. loading both ".css" and the inlined CSS in JS) for legacy mode:
On initial page load, i.e. SSR, just serve 1, like happens today. |
No, I'm not suggesting that. I'm just saying that I guess that behavior is not handled by Vite. For a client side routing app that is chunk splitted at dynamic imports, Vite will preload CSS before loading that chunk. |
What happen today is that preloading CSS, together with preloading the JS in general, isn't happaning at all on legacy mode (and PR #9920 changes this behaviour). TD'LR: In modern mode everything just works, in legacy mode not everything works. This seems to be a wrong behavior overall as far as I understand. |
I formed a suitable issue for out latest discussion in #10285. The continuation of this conversation can be moved there. |
Description
simple build will collect all entry module css into chunk.viteMetadata.importedCss and inject into the
index.html
same with legacy.legacy build should avoid insert the entry module css again
the test would same with #9507.
Sync chunk no generate the css but also had the style.
Async chunk should generate the css and had the style.
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).