barrel / shopify-vite

Modern frontend tooling for Shopify theme development using Vite for a best-in-class DX.
https://shopify-vite.barrelny.com/
MIT License
293 stars 45 forks source link

Add support for defer css loading #136

Closed tuanpham-barrel closed 4 months ago

tuanpham-barrel commented 5 months ago

Description

This pull request introduces enhancements to the CSS loading mechanism to improve page load performance by deferring non-critical CSS. This approach follows best practices as discussed in this article on deferring non-critical CSS.

Changes include:

Impact:

These changes allow for better prioritization of resource loading and can significantly enhance user experience by speeding up the perceived load time of web pages.

netlify[bot] commented 5 months ago

Deploy Preview for bespoke-flan-5eacfc canceled.

Name Link
Latest commit 9820c7f036df7fefea19f6317227e80b519569fe
Latest deploy log https://app.netlify.com/sites/bespoke-flan-5eacfc/deploys/667ac8749aa50c0008c94719
montalvomiguelo commented 5 months ago

Thanks a lot for sharing this @tuanpham-barrel 🙏

@wturnerharris and I tried multiple approaches for critical CSS in the past. I recall the Visionist project; generating and updating that critical CSS as we released updates was a complex process. I would not recommend it for any project anymore, and I would not like to add it to the core of Shopify Vite Plugin.

An alternate approach to eliminate render-blocking stylesheets and improve performance is to leverage resource hints using the preload attribute of the stylesheet_tag filter. With that in mind, I added the preload_stylesheet parameter in Shopify Vite Plugin.

Here is a quick test with no performance problems with stylesheets using resource hints.

Please give it a try and let me know what you think (:

tuanpham-barrel commented 5 months ago

@montalvomiguelo - Do you think it's over-engineered?

montalvomiguelo commented 5 months ago

Yes, it is. ~Also, as I mentioned earlier, generating and keeping those critical styles up-to-date could be a complex task.~

The alternate approach with resource hints I shared earlier is a better way to solve potential problems when loading stylesheets.

I strongly encourage you to try it and benchmark. If you have many style sheets on the page, keep an eye on Style recalculations.

If you still need this functionality, let's isolate it in its own plugin? 🙏

tuanpham-barrel commented 5 months ago

@montalvomiguelo

This is simply for defer loading non-critical css not for generating critical css.

I don't think using resource hints approach is a good approach here. Because it will do in opposite way that preload non-critical css while we need to load it later.

montalvomiguelo commented 5 months ago

@tuanpham-barrel If we loaded one single stylesheet with the styles of the whole store and used resource hints to prevent rendering blocking resources, there would be no negative impact on performance. Here is a quick test with a perfect 98 score mobile. I want to keep this as a default for the core plugin. Worth exploring that path in your specific use case?

The deferred CSS functionality may not be necessary. Let's keep it optional and isolate it in its own plugin? I don't want code in the core plugin that will be rarely used. E.g., theme store developers go through a rigorous review process, and I want to avoid this deferred CSS functionality potentially impacting them.

tuanpham-barrel commented 5 months ago

That's your test for small amount of resources. There are many other resources need to be preloaded like fonts, images and deferred css should not be preloaded or block the rendering.

Also, this approach was done in theme that's in theme store. I don't think there is any problem with it.

montalvomiguelo commented 5 months ago

@tuanpham-barrel The test does preload other resources like fonts and images, but those are separate concerns. Regarding the stylesheet, you can see that it is one (like in many themes) and non-blocking, with 0 problems to solve and 0 benefits of deferring CSS.

Support for deferring CSS should be optional via Plugin, as it might not be necessary in most cases. Unneeded/unused liquid code is a problem; Shopify’s theme review team is picky.

Please share some tests of potential wins here 🙏

pablogiralt commented 4 months ago

@tuanpham-barrel like you, I want to defer certain stylesheets. How are you approaching the issue currently?

@montalvomiguelo being able to defer certain stylesheets makes much easier to bring existing themes that use a stylesheet per section to Vite

montalvomiguelo commented 4 months ago

@tuanpham-barrel Can you finish the plugin for this functionality?

tuanpham-barrel commented 4 months ago

@pablogiralt - I ended up using another snippet asset.liquid as a wapper for vite-tag.liquid. You can check the code below. All you need to do is replacing render 'vite-tag' with render 'asset'.

{%- comment -%}
  Parameters
  * asset
  * preload_stylesheet
  * fetch_priority
{%- endcomment -%}

{%- liquid
  assign href = blank
  assign extension = asset | split: '.' | last

  if preload_stylesheet == 'defer' and extension == 'css'
    capture output
      render 'vite-tag' with asset, preload_stylesheet: false
    endcapture

    assign href = output | split: 'href="' | last | split: '"' | first
  endif
-%}

{%- if href != blank -%}
  <link
    rel="preload"
    fetchpriority="{{- fetch_priority | default: 'low' -}}"
    href="{{- href -}}"
    as="style"
    onload="this.onload=null;this.rel='stylesheet'"
  >
  <noscript><link rel="stylesheet" href="{{- href -}}"></noscript>
{%- else -%}
  {%- render 'vite-tag' with asset, preload_stylesheet: preload_stylesheet -%}
{%- endif -%}
pablogiralt commented 4 months ago

Awesome tuanpham-barrel!