Add AssetPreload check #603
Conversation
docs/checks/asset_preload.md
Outdated
| ### ✗ Fail | ||
|
|
||
| ```liquid | ||
| <link href="style.css" rel="preload" as="style"> |
There was a problem hiding this comment.
Nit: a storefront would typically use <link href="{{ 'style.css' | asset_url }}" rel="preload" as="style"> here. You can't really have assets as relative paths.
| return if node.attributes["rel"]&.downcase != "preload" | ||
| case node.attributes["as"]&.downcase | ||
| when "style" | ||
| add_offense("Prefer preload argument on stylesheet_tag filter", node: node) |
There was a problem hiding this comment.
Minor nit:
For better performance, prefer using the preload argument of the stylesheet_tag filter
There was a problem hiding this comment.
(and something similar for the other ones)
krzksz
force-pushed
the
asset-preload-check
branch
from
be6420b to
3a31dff
Compare
| END | ||
| ) | ||
| assert_offenses(<<~END, offenses) | ||
| Prefer preload argument on stylesheet_tag filter at templates/index.liquid:1 |
There was a problem hiding this comment.
Those tests should be failing considering the error message change, shouldn't they?
|
Hmm why are the unit tests not running? 🤔 Oh. @krzksz, it's because you haven't used the Shopify remote when pushing your changes (you used a fork) and we probably have setup this repo to run unit tests only by employees. We should change that... or figure out a better way. |
|
Closing in favour of #605 |
Hey 👋
The goal of this check is to discourage people from preloading assets using plain HTML in cases when it is possible to use Liquid filters instead.
It is sometimes the case that
<link rel="preload" ...>is positioned late in the<head>, preventing the browser from discovering it early. In addition, this manual approach is not automatically converted toLinkheader so Shopify is not able to forward it to Early Hints response. These issues are not present when using Liquid filters mentioned above.