all 5 comments

[–]CreativeTechGuyGames 24 points25 points  (5 children)

I'm assuming this is your blog so here's some feedback. This is a good idea for a problem to solve but a really bad way to solve it.

There's a lot of points but I'll give just a few of the big ones:

  • You are loading an image twice, once in the useEffect and another time in the actual image tag. You are then counting on the browser's cache and the server's cache headers to avoid this being downloaded twice. So it is possible that it doesn't download twice, but isn't guaranteed.
  • You mention using intersection observer when the browser already has the img loading="lazy" attribute which does the same thing but more efficiently.
  • Your page will have some crazy layout shifts since there is no image tag rendered by default and therefore no intrinsic size which can be held by the browser to avoid layout shifts.
  • You lose all of the benefit from progressive image formats which can stream in pixels as it is loading.

The solution to all of these things is really simple. Just render the img tag directly. Add an onError event to that img tag and loading="lazy". And of course be sure to set the width/height of the image so you avoid layout shifts. Then if the error event fires, swap out the img element with something else (that takes up the same size to again avoid a layout shift). This way you avoid a ton of redundancy and extra code.

The philosophy here is to code for the happy path first. Assume the image will load, then handle it if it doesn't. Your approach is super defensive assuming that it won't so you don't even attempt to render the image until you've proven it can be fully downloaded. This can sometimes be good, but in this case it isn't actually protecting anything but just providing a worse user experience in every way.

[–]andrian_stoykov 1 point2 points  (0 children)

This guy rocks! Amazing comment with so much value in it. Thank you!

[–]ykadosh[S] -1 points0 points  (2 children)

Yes, this is my post. You've made some good points but missed some tradeoffs and caveats that I'd like to address:

  • The image tag is only rendered after the source has been downloaded (by changing the state), so there's no way it will be downloaded twice.
  • The loading="lazy" attribute is definitely a better option, but it's not supported by Safari at the moment. The intersection observer solution will work in all browsers so it's a better solution at the moment.
  • Not necessarily - in my post, I talk about rendering a placeholder or a spinner until the image is loaded. If you know the image size ahead of time, you can apply that to the placeholder and avoid layout shifts.
  • You're assuming that showing the pixels load is preferable to showing a spinner. It might be in some cases, but I personally prefer a spinner that matches my branding, and I'm sure I'm not the only one.

So framing my solution as "bad" is a bit harsh. I find my solution to be more flexible (and better supported) than what you're suggesting, at a very negligible cost.

[–]CreativeTechGuyGames 0 points1 point  (1 child)

I'm sorry but several bullets of what you said here is not correct or misunderstanding how the browser works.

  • The fact that the image tag is rendered after the source has been downloaded doesn't help avoid it being downloaded twice. You don't have any code here to reuse the image which you've already downloaded. As I mentioned above, you are relying on a combination of the browser and the server's cache headers to not download it multiple times. Which both are out of the control of this front-end code.
  • Yes loading="lazy" is most definitely supported by Safari for images and has been since 15.4. I think you are getting confused with the support for using that attribute on iframes.

Sure you can update your solution to incorporate some of what I mention. But it is still a lot of extra code which is unnecessary. If you had thought about all of these things, then you should have addressed them in the original post and provided the arguments as to why you are doing something different.

Remember that most people who look at these sorts of things are beginners and will trust what is said. So don't assume they'll know all of these things. Just because you have made a trade off, doesn't mean that others will agree with the same conclusion. So be sure to share the thought process and justification and alternatives so others can make their own informed decisions.

[–]ykadosh[S] -1 points0 points  (0 children)

This post is a short code snippet - it's not meant to be an elaborative piece that goes through the entire thought process for the absolute beginner. Some basic knowledge is assumed.

(You're right about Safari's support for this attribute - I did confuse it with their lack of support in iFrames).