search

tastejs / nuxt-movies

Nuxt.js Movies - a TMDB client optimized for Core Web Vitals
https://nuxt-movies.vercel.app/
73 stars 24 forks source link

[perf] use properly size images by srcset and sizes attributes #24

Closed anton-karlovskiy closed 2 months ago

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nuxt-movies/nuxt-movies/1wwou57ve
✅ Preview: https://nuxt-movies-git-fork-anton-karlovskiy-perf-properly-size-images.nuxt-movies.vercel.app

anton-karlovskiy commented 3 years ago

@addyosmani

FYI: please do not merge this PR as it's in progress. :) Thank you.

addyosmani commented 3 years ago

(Minor note in passing) is this a good opportunity to experiment with https://image.nuxtjs.org/nuxt-image/?

anton-karlovskiy commented 3 years ago

(Minor note in passing) is this a good opportunity to experiment with https://image.nuxtjs.org/nuxt-image/?

@addyosmani Thank you for your input.

Yes, I'm planning to do with it. But for now, I took a direction to go with the native lazy-loading and responsive images approach and actually have achieved a lot of improvement in performance. So I'm going to complete in this direction in the first place and then experiment with https://image.nuxtjs.org/nuxt-image/ as well to compare the two cases.

What do you think?

cc @pi0

pi0 commented 3 years ago

@addyosmani Fully agree to use nuxt/image (see #26). (docs are not updated with recent changes)

If I'm correct idea of this project is to evaluate a realistic nuxt application performance. We can of course use manual optimizations like for image srcset but i'm afraid this will make demo out of nuxt scope to best practices all websites should have...

@anton-karlovskiy I think we might introduce an option for image module to disable use of IntersectionObserver for <nuxt-image loading="lazy" to compare but then again numbers won't be realistic since we finally do need to consider Safari users and default options for image module are something we recommend to them.

anton-karlovskiy commented 3 years ago

@pi0

Thank you for letting me know. I'd like to listen to @addyosmani before dropping this srcset option. I just thought that the current srcset implementation would be used together with https://image.nuxtjs.org/nuxt-image/. Please correct me if I'm wrong. Thank you.

pi0 commented 3 years ago

@anton-karlovskiy Finally having srcset would be best. But image module will generate provider links (resized) instead of manually computing. I.e. this PR is perfect but not reflecting nuxt perf if that is final goal of this project :)

anton-karlovskiy commented 3 years ago

@pi0

Thank you for explaining. I think we might have to listen to @addyosmani's opinion then.

I think we can leave this PR aside if it's very generic and start from your PR if we want to focus on nuxt perf. :)

addyosmani commented 3 years ago

I wanted to leave a general comment on how we should approach optimizations for this project. The overall goal is not to create an application that has perfect performance using manual methods, but to:

  1. Do what the typical Vue.js or Nuxt.js developer would do to solve the problem (in this case, we should use nuxt-image and see what gaps it has for better perf)
  2. Once we have done this, look at what hacks are necessary in order to get better performance (i.e what can the Nuxt team to in order to improve their defaults)

How can we approach this?

When we see that there's an optimization the app needs, we should ask @pi0 or @Atinux what a typical Nuxt.js user would do to solve the problem. Is there a specific plugin they would use? A specific flag? We should use that approach and then document things like [1] what didn't work, [2] what was hard etc. I think this will ensure that the work we're doing here is of most value to the Nuxt team.

Does that make sense?

addyosmani commented 3 years ago

(That's to say that let's finish comparing this approach vs #26 briefly, but move ahead with #26 as soon as we can. There's plenty more than can be optimized for the app... :))

addyosmani commented 3 years ago
I think we can leave this PR aside if it's very generic and start from your PR if we want to focus on nuxt perf. :)

This sgtm. If we can switch over to #26 now, we can always come back to this PR if we feel that there are gaps in nuxt-image that require us to go down the custom implementation path.