search

vitejs / vite

Next generation frontend tooling. It's fast!
http://vite.dev
MIT License
68.76k stars 6.21k forks source link

Non-modern build should also ("pre")load CSS #9902

Open Tal500 opened 2 years ago

Tal500 commented 2 years ago

Description

While I'm doing my efforts in sveltejs/kit#6265 to add legacy support to SvelteKit on the Vite way, I had noticed that when navigating from one page to the other, the internal function __vitePreload is called, which is defined in the plugin importAnalysisBuild.

As far as I understand, the purpose of this function is to preload JS modules and also to (pre?)load CSS files. On legacy output chunks, it shuts down the preloading ability. I understand that there is no need to preload JS modules on non-modern(i.e. non ESModule compatible) browsers, but for the correct way of loading CSS, the CSS files should(?) be loaded in __vitePreload.

According to my local tests, if the CSS are getting preloaded on legacy builds, everything works great in my legacy work on SvelteKit.

Suggested solution

Change the code of the plugin importAnalysisBuild to (pre?)load CSS anyway, and when the browser is also modern, import also the preloaded JS files.

Alternative

You tell me?

Additional context

No response

Validations

fejiroofficial commented 2 years ago

Hello Tal500. I appreciate your effort put in here. I was trying out your solution on our code, but the production build was failing due to this: Is this issue in any way related? [vite-plugin-svelte-kit] Prerendering failed with code 1 error during build: Error: Prerendering failed with code 1 at ChildProcess.<anonymous> (file:///Users/fejirogospel/Documents/GitHub/center-nx/apps/site-app/node_modules/@sveltejs/kit/src/exports/vite/index.js:455:14) at ChildProcess.emit (node:events:527:28) at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)

Tal500 commented 2 years ago

Hello Tal500. I appreciate your effort put in here. I was trying out your solution on our code, but the production build was failing due to this: Is this issue in any way related? [vite-plugin-svelte-kit] Prerendering failed with code 1 error during build: Error: Prerendering failed with code 1 at ChildProcess.<anonymous> (file:///Users/fejirogospel/Documents/GitHub/center-nx/apps/site-app/node_modules/@sveltejs/kit/src/exports/vite/index.js:455:14) at ChildProcess.emit (node:events:527:28) at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)

Hello! Are you talking about the SvelteKit legacy PR I made? If so, you better comment this issue on the PR I sent on SvelteKit, not here.(was it a mistake?)

Anyway, for your case, can you:

  1. Disable prerendering (in svelte.config.js), do npm run build && npm run preview, and share the result? Also, can you please publish a bigger chunk of the console error? It's seems that a detailed error message appears earlier.
  2. Can you give a sample code for reproduction?
  3. Did you try to compile and run the simple demo?

Please comment to me to this on the PR in SvelteKit, for a better discussion.

Tal500 commented 2 years ago

Update: After working on PR #9920, I've changed my opinion - I think that on legacy builds we should have full preloading support.

Tal500 commented 2 years ago

After some investigation, I realized that Vite always inline the legacy CSS in injected HTML code. But why?

@yyx990803 , what is the reason for 940d48333f6572314576da9312f36018fd70bdc8 ? Wouldn't it be nicer to also load the CSS normally, the same way you load it in modern browsers? (any technical difference between modern and legacy here in CSS?)

sapphi-red commented 2 years ago

It's described here. https://github.com/vitejs/vite/issues/2062#issuecomment-782385406

Tal500 commented 2 years ago

It's described here. #2062 (comment)

Thank you for the reference!

So as far as I see, there is no principle from Vite maintainers for not preloading content on legacy, right? This can easily solves all the problem, and will allow us to get rid of inlining css issues on legacy🙂

My PR #9920 does exactly this. If we want preloading on legacy, we can get rid of all the CSS inlining mess (update: this PR got rid of it).

Did I get it right? Do you agree we shall do it?

sapphi-red commented 2 years ago

IIUC Vite inlines CSS to realize preloading CSS by using JS chunks instead. The reason why it isn't using the __vitePreload seems to be because linkElement.onload does not work in legacy browsers.

https://github.com/webpack-contrib/mini-css-extract-plugin/issues/294 https://pie.gd/test/script-link-events/

Tal500 commented 2 years ago

IIUC Vite inlines CSS to realize preloading CSS by using JS chunks instead. The reason why it isn't using the __vitePreload seems to be because linkElement.onload does not work in legacy browsers.

webpack-contrib/mini-css-extract-plugin#294 https://pie.gd/test/script-link-events/

OK, I read the sources, so now I understand the "XHR request" meaning on https://github.com/vitejs/vite/issues/2062#issuecomment-782385406 .

So I will simply make such a request on legacy, and it will just "embed" the content of the CSS file between <style>...</style> tags. There will be an attribute data-href={original_source} on the style tag for future detection, to prevent loading the same stylesheet twice.

Sounds good?

sapphi-red commented 2 years ago

I think that works for most cases but it won't for a CSS file containing relative paths (background: url('./foo.png')). The relative paths are relative to CSS file, but when injected into <style> tag, it will be relative to location.href.

I'm not sure but an approach like https://guybedford.com/es-module-preloading-integrity might work.

Tal500 commented 2 years ago

I think that works for most cases but it won't for a CSS file containing relative paths (background: url('./foo.png')). The relative paths are relative to CSS file, but when injected into <style> tag, it will be relative to location.href.

I'm not sure but an approach like https://guybedford.com/es-module-preloading-integrity might work.

How about do (in parallel):

  1. Inject the link tag after
  2. Query XHR to fetch(and cache) and see if the resource available, see also errors we're getting.

Do you think that the browser will execute the query twice? It seems that the caching should be immediate, but I'm not sure. It's kinda like the loading to image tag trick and then on error we know that the resource fetching has been finished.

Or to calculate correctly how to get the correct path.

We can also "give up" on the detection of error in CSS path on legacy, and then use the trick of loading to image and then to link.

Tal500 commented 2 years ago

I think that works for most cases but it won't for a CSS file containing relative paths (background: url('./foo.png')). The relative paths are relative to CSS file, but when injected into <style> tag, it will be relative to location.href.

I'm not sure but an approach like https://guybedford.com/es-module-preloading-integrity might work.

I had updated PR #9920 to do this things. Tell me what do you think about the new preload function: https://github.com/vitejs/vite/blob/e72c0905bec8b0540fa41622956d26028beed04e/packages/vite/src/node/plugins/importAnalysisBuild.ts#L58-L169

mreduar commented 1 year ago

Any update here?