zerodevx / svelte-img

High-performance responsive/progressive images for SvelteKit
https://zerodevx.github.io/svelte-img/
ISC License
321 stars 11 forks source link

feat: fetchpriority #42

Open leoj3n opened 7 months ago

leoj3n commented 7 months ago

I don't see any support for fetchpriority="high" here. I'm thinking this is something you may want to add.

To use priority hints with picture tags simply add the fetchpriority attribute to the img element inside the picture tag.

<picture>
  <source srcset="/image-small.png" media="(max-width: 600px)">
  <img src="/image.png" fetchpriority="high">
</picture>

https://www.debugbear.com/blog/priority-hints#picture-elements-and-priority-hints

Take a Largest Contentful Paint image, which, when preloaded, will still get a low priority. If it is pushed back by other early low-priority resources, using Fetch Priority can help how soon the image gets loaded.

https://web.dev/articles/fetch-priority#summary

zerodevx commented 7 months ago

Right now unknown component props are forwarded to the internal <img> tag, so:

<Img {src} fetchpriority="high" />

will be rendered similar to your attached code - but not for media attribute though. Most of the time (I think), one could use sizes to pick up the correct image instead.

leoj3n commented 7 months ago

@zerodevx I thought you might reply along these lines. I would point out that other props you have implemented fall under the same caveat. Not sure what is the best way forward, but as long as you have implemented export let loading = 'lazy' and export let decoding = 'async' (which is far less useful) I'm not sure you can argue not to include this.

Edit: The media attribute is from where I copy-pasta the example. I have not needed to use media= myself yet.

zerodevx commented 7 months ago

The reason why loading and decoding are added is because they change <img> tag defaults - IMO unless we're changing fetchpriority defaults there isn't a strong case to define it explicitly?

leoj3n commented 7 months ago

Can you give an example of code output? Not understanding what you mean by changing img tag defaults.

zerodevx commented 7 months ago

I meant the HTMLElement <img> tag default values; specifically loading and decoding for which we are overriding the default value. Hence:

https://github.com/zerodevx/svelte-img/blob/8254cd1994b3ff20b30b72392c6f40a55ab360ad/src/lib/SvelteImg.svelte#L5-L18

These explicitly declared props are not arbitrary - each of them has a reason behind why it's declared.

I'm not understanding the reason for explicitly declaring fetchpriority at the moment - unless you are recommending to override the default value.

leoj3n commented 7 months ago

@zerodevx I see what you are saying. My original thought was to include it for educational purposes for the users of svelte-img. I would have it be unset, so by default the browser can still decide via it's heuristics the best priority.

Off-topic: I guess it's an OK idea to always default to lazy for loading, but I'm not so sure it's needed to set decoding.

Anyways I think it is nice to be explicit and mention all the different toggles that are available that one will probably want to use with this component, at the very least in the docs. Me from a couple weeks ago might have just picked up this library and ran with it without coming across fetchpriority/loading/decoding. You might say it's not your job / the job of this repo to educate but I would argue a lib like this is ground zero for someone looking for a quick solution and it is helpful place to give some advice. But I understand if you think differently and prefer to keep more of a laser focus here.

If you were to make a typedef and add these to that it would show up in the in the IDE which might be nice. I suppose in that case you would be tempted to extend HTMLImgAttributes which kind of obscures these critical options again. So, at least a note in the README would be nice... But I won't be surprised if you are of the opinion that people should find out about these options elsewhere.

Feel free to close; just am curious to hear how you think about this.

zerodevx commented 6 months ago

mention all the different toggles ... at the very least in the docs

That could be a lot of props. 😅 But I think we could mention the salient ones in the docs. Will be delighted to take a PR for it.

If you were to make a typedef...

That's a good alternative too - don't have to hint every prop, but salient ones like what you mentioned might be helpful.

prefer to keep more of a laser focus

Generally yes - I'd normally lean towards providing just a base set of opinions, but allowing effects to be overridden if the consumer is more informed or inclined to do so.

leoj3n commented 6 months ago

That could be a lot of props.

I'm pretty sure I was thinking fetchpriority/loading/decoding as "toggles" people should know about what others were you thinking?

I will submit a PR later with some types with comment that would show up in IDE when using the component.