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

Entry points doesn't inline CSS, but client-side navigation might load another entry point #10285

Closed
7 tasks done
Tal500 opened this issue Sep 29, 2022 · 4 comments · Fixed by #10496 · May be fixed by #9920
Closed
7 tasks done

Entry points doesn't inline CSS, but client-side navigation might load another entry point #10285

Tal500 opened this issue Sep 29, 2022 · 4 comments · Fixed by #10496 · May be fixed by #9920
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) plugin: legacy regression The issue only appears after a new release

Comments

@Tal500
Copy link
Contributor

Tal500 commented Sep 29, 2022

Describe the bug

I'm just forming the discussion started at #9761 (comment) in a hopefully well formed issue.

This issue is somewhere between a bug and a feature, the best way to see this is as "behavior that was fine and now was stopped working"(after PR #9761).

Problem

Since PR #9761, client side navigation on legacy mode is problematic.
For example, assume hypothetically you have a Nuxt website with the pages "/"(homepage) and "/about", each of them having some CSS.
The user may start in the page "/" and then navigate(**in the client side) to the page "/about", or the opposite, each of the option should be totally valid.
According to Vite, AFAIK, both of the pages are considered to be entry points.
If we're not on legacy mode, everything works well, since both on SSR and client-side navigation, the client always load the relevant <link> tags of the CSS files needed.

Assuming we're on legacy mode now.
On the first page the user load, there's no need to load JS-inlined CSS, since by SSR there are <link> tags referencing to the relevant CSS files. This was the motivation of PR #9761, that canceled CSS-inlining by JS for entry points, since they are already loaded by SSR.
But on our case, the SSR loads initially only the CSS files for the first page, but not to the second one.
When the user use the client side navigation to the second page, neither of the CSS files or the JS-inlined CSS is loaded! The former doesn't get loaded because we're on legacy mode, and the second isn't loaded because of PR #9761.

Solutions

As the last paragraph suggests, we have three possible solutions:

  1. Loading the CSS files like in modern mode. This is the most elegant solution in my opinion, and was done in PR feat: support preloading also on legacy bundle #9920.
  2. Revert PR perf: legacy avoid insert the entry module css #9761, which then will inline the CSS always on legacy, no matter if it's entry point or not. The problem of course is the original issue it solved, that the JS output will be larger since it contains the inlined-CSS, which is might be duplicated since the CSS files might be already loaded on SSR. Therefore, I suggest also the following next solution.
  3. Perform the CSS-inlining in a different JS file for each entry points. This way, on Vite preloading(which is essentially the Vite client-side navigation ability) in legacy mode, Vite will check if the CSS assets where already loaded, and only if not, Vite will import the JS file representing the relevant CSS-inlining. This way there the client will never (at the same session) need to load the same asset twice, once as CSS and once as JS.

Reproduction

See the code and the online preview in the readme desctiption(you need to use legacy browser for see the issue of CSS missing on navigation): https://github.com/Tal500/sveltekit-legacy-demo#online-preview

System Info

Not relevant.

Used Package Manager

npm

Logs

No response

Validations

@Tal500
Copy link
Contributor Author

Tal500 commented Oct 12, 2022

@sapphi-red @poyoho

Any update on the decision made on this issue?
It's a real blocker for sveltejs/kit#6265 for a long time, and can be fixed easily by #9920 that is also getting old.

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release labels Oct 17, 2022
@sapphi-red
Copy link
Member

I now understand when this happens.
https://stackblitz.com/edit/vitejs-vite-gdwexk?file=dist%2Fassets%2Fcounter.e7e030da.js&terminal=dev
This happens when an entry file is imported by a dynamic import.

IMO we should revert #9761 as it's a regression.
#9920 will take a bit more time as we haven't discussed this yet.
About the 3rd solution, I don't find any difference from #9920. But maybe I'm missing something.

@poyoho
Copy link
Member

poyoho commented Oct 17, 2022

I think revert #9761 it seems the best chose. 👍

@Tal500
Copy link
Contributor Author

Tal500 commented Oct 17, 2022

Tnx for fixing this issue!

About the 3rd solution, I don't find any difference from #9920. But maybe I'm missing something.

To be clear - what I mean in the 3rd solution is that we can put the JS-inlined-CSS in a different JS file, and load in on __vitePreload() only if the matching CSS wasn't already exists as a <link> node in the DOM (this can happen when the matching CSS file is loaded in the <head> node during SSR).

The advantage of 3rd solution over the 2nd is that the client wouldn't need to fetch a JS file containing the inlined CSS if the CSS file were already loaded (say, by SSR), and this reduces the number of downloaded bytes on HTTP.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) plugin: legacy regression The issue only appears after a new release
Projects
None yet
3 participants