beekai-oss / react-simple-img

🌅 React lazy load images with IntersectionObserver API and Priority Hints
MIT License
989 stars 41 forks source link

Invalid empty srcSet attribute in SSR #85

Closed ZLevine closed 4 years ago

ZLevine commented 4 years ago

Describe the bug We're attempting to use this library with Gatsby. In SSR (server-side rendering), an empty srcSet="" attribute is rendered on the <img tag. This is bad because:

a) an empty srcset attribute is invalid, and b) This may cause the browser to block rendering and attempt to load the img based on srcset, completely defeating the purpose of image-lazyloading.

Here's an example of the minified output:

<img style="height:100%;width:100%;visibility:hidden;position:absolute;top:0;left:0" alt="retropie" src="data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs=" srcSet="" data-src="https://howchoo.com/media/mt/a5/yz/mta5yzzjoweg_md.jpeg" data-srcset="https://howchoo.com/media/mt/a5/yz/mta5yzzjowet.jpeg 200w, https://howchoo.com/media/mt/a5/yz/mta5yzzjoweg_xs.jpeg 360w, https://howchoo.com/media/mt/a5/yz/mta5yzzjowem.jpeg 414w, https://howchoo.com/media/mt/a5/yz/mta5yzzjoweg_md.jpeg 768w, https://howchoo.com/media/mt/a5/yz/mta5yzzjoweg.jpeg 900w, https://howchoo.com/media/mt/a5/yz/mta5yzzjowel.jpeg 1440w" sizes="(max-width: 200px) 200px, (max-width: 360px) 360px,(max-width: 414px) 414px, (max-width: 768px) 768px,(max-width: 900px) 900px, (max-width: 1440px) 1440px, 900px"/>

Note srcSet="" above.

To Reproduce Steps to reproduce the behavior:

  1. Use react-simple-img normally
  2. Run SSR (for gatsby, using gatsby build && gatsby serve)
  3. Observe the HTML output contains an empty srcSet="" attribute on the <img> tag.

Expected behavior I expect that attribute to not exist in the SSR HTML.

Screenshots N/A

Desktop (please complete the following information):

Thanks for the excellent library! I'd be happy to help test a fix if needed.

jhpmatos commented 4 years ago

+1

bluebill1049 commented 4 years ago

any chance you can create a PR for this? I am pretty busy with another OS project at the moment.

bluebill1049 commented 4 years ago

you want to give a try this version: "version": "2.3.8-beta.1",

ZLevine commented 4 years ago

My apologies, I thought GitHub would notify me of all comments since I created this issue—I only received the Close notification.

I tested that branch and it's not quite fixed; now, the full srcSet value is output in SSR. Instead, it should be omitted entirely in SSR.

Here's the result of that branch, broke out for clarity:

<img
    style="height:100%;width:100%;visibility:hidden;position:absolute;top:0;left:0"
    alt="How to securely empty trash by default in Mac OS X"
    src="data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs="
    srcSet="https://howchoo.com/media/yw/u3/yz/ywu3yza4owmt.png 200w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg_xs.png 360w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmm.png 414w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg_md.png 768w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg.png 900w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owml.png 1440w"
    data-src="https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg.png"
    data-srcset="https://howchoo.com/media/yw/u3/yz/ywu3yza4owmt.png 200w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg_xs.png 360w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmm.png 414w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg_md.png 768w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg.png 900w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owml.png 1440w"
    sizes="(max-width: 200px) 200px, (max-width: 360px) 360px,(max-width: 414px) 414px, (max-width: 768px) 768px,(max-width: 900px) 900px, (max-width: 1440px) 1440px, 900px"
/>

What it's doing

A full srcSet attribute and value are being output; this would negate lazy-loading as the browser would attempt to load the image once it finds a valid srcset attribute.

What it should be doing:

In SSR, the srcSet attribute should be missing entirely. This way, it can be added when the image is lazy-loaded. For example, it should look like this:

<img
    style="height:100%;width:100%;visibility:hidden;position:absolute;top:0;left:0"
    alt="How to securely empty trash by default in Mac OS X"
    src="data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs="
    data-src="https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg.png"
    data-srcset="https://howchoo.com/media/yw/u3/yz/ywu3yza4owmt.png 200w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg_xs.png 360w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmm.png 414w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg_md.png 768w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owmg.png 900w, https://howchoo.com/media/yw/u3/yz/ywu3yza4owml.png 1440w"
    sizes="(max-width: 200px) 200px, (max-width: 360px) 360px,(max-width: 414px) 414px, (max-width: 768px) 768px,(max-width: 900px) 900px, (max-width: 1440px) 1440px, 900px"
/>

I'll make sure I get notifications for this issue and check regularly if you'd like me to test another branch. Thanks!

ZLevine commented 4 years ago

I submitted a PR with a fix. By setting the srcSet prop value to null if it's not cached, the prop is omitted in SSR HTML output but functions correctly in the client. isCached will always return false if it's not a browser since its state value is defaulted to false :)

bluebill1049 commented 4 years ago

awesome thanks @ZLevine I will release the patch soon.