Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fetchpriority="high" when next/image has priority prop #40887

Conversation

sanjaiyan-dev
Copy link
Contributor

Eagerly load images on viewport 🖼️

When user scroll to bottom of the website so swiftly (which have lot of images) currently Next.js marks even other image also needed to be fetched which might cause delay of loading of current image which is located in user's current position.
So, we can add priority hint to the particular image element in the viewport to load in high priority.

Extremely sorry my explanation might not be good, hope you got what I am trying to say 😆

Sorry if I made any mistakes :(

Copy link
Contributor

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the WICG Priority Hints draft:

With in-viewport images, a browser may automatically detect if the image is important and boost priority if it gets to layout early enough.

As the draft described, the browser could have detected the in-view image and automatically boost the fetch priority, it is probably unnecessary to dynamically set the fetchpriority.

Instead of dynamically setting fetchpriority to high when intersected and removing the fetchpriority when out of the viewport, personally I'd prefer setting a static fetchpriority.

next/image component already accepts the priority={boolean} prop. Would you like to change the implementation so that the fetchpriority="high" is set when priority={true} is specified?

@sanjaiyan-dev
Copy link
Contributor Author

According to the WICG Priority Hints draft:

With in-viewport images, a browser may automatically detect if the image is important and boost priority if it gets to layout early enough.

As the draft described, the browser could have detected the in-view image and automatically boost the fetch priority, it is probably unnecessary to dynamically set the fetchpriority.

Instead of dynamically setting fetchpriority to high when intersected and removing the fetchpriority when out of the viewport, personally I'd prefer setting a static fetchpriority.

next/image component already accepts the priority={boolean} prop. Would you like to change the implementation so that the fetchpriority="high" is set when priority={true} is specified?

Understood :)

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to change the implementation so that the fetchpriority="high" is set when priority={true} is specified

Let's not do that without measuring the impact on performance. The last I checked, the fetchpriority prop is not as good as the current priority preloading prop.

@sanjaiyan-dev
Copy link
Contributor Author

Would you like to change the implementation so that the fetchpriority="high" is set when priority={true} is specified

Let's not do that without measuring the impact on performance. The last I checked, the fetchpriority prop is not as good as the current priority preloading prop.

https://web.dev/priority-hints/#increase-the-priority-of-the-lcp-image

@sanjaiyan-dev
Copy link
Contributor Author

sanjaiyan-dev commented Oct 2, 2022

@sanjaiyan-dev
Copy link
Contributor Author

Would you like to change the implementation so that the fetchpriority="high" is set when priority={true} is specified

Let's not do that without measuring the impact on performance. The last I checked, the fetchpriority prop is not as good as the current priority preloading prop.

Additional ref-: https://angular.io/guide/image-directive#marking-images-as-priority

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the fetchPriority prop need to be added to the preload <link> in the head?

@styfle styfle changed the title Eagerly load images on viewport 🖼️ Add fetchpriority=high when next/image has priority prop Oct 12, 2022
@sanjaiyan-dev
Copy link
Contributor Author

Does the fetchPriority prop need to be added to the preload <link> in the head?

@styfle I think no need according to implementation of angular.

@styfle
Copy link
Member

styfle commented Oct 12, 2022

Looks like CI is failing due to missing TS types

Also, needs updated tests to ensure the prop is working correctly

@sanjaiyan-dev sanjaiyan-dev changed the title Add fetchpriority=high when next/image has priority prop Add fetchpriority="high" when next/image has priority prop Oct 13, 2022
@balazsorban44
Copy link
Member

@sanjaiyan-dev is there an intention to push this forward? Currently, there are merge conflicts and there are unaddressed comments here: #40887 (comment)

@sanjaiyan-dev
Copy link
Contributor Author

@sanjaiyan-dev is there an intention to push this forward? Currently, there are merge conflicts and there are unaddressed comments here: #40887 (comment)

Yh, I will fix it ASAP 🤝🙌

@styfle
Copy link
Member

styfle commented Oct 22, 2022

How does this affect performance of stylesheets? Do we need to configure fetchPriority on those too?

@sanjaiyan-dev
Copy link
Contributor Author

How does this affect performance of stylesheets? Do we need to configure fetchPriority on those too?

Extremely sorry I am not sure about it but I created PR related loading of first party scripts and optimized way of preloading.

#41084

#41423

@ijjk ijjk force-pushed the canary branch 3 times, most recently from df8579c to 47e5ebe Compare October 25, 2022 16:15
@sanjaiyan-dev
Copy link
Contributor Author

Let's Start from scratch :)

@sanjaiyan-dev
Copy link
Contributor Author

@sanjaiyan-dev is there an intention to push this forward? Currently, there are merge conflicts and there are unaddressed comments here: #40887 (comment)

I have created refined PR 🤝

#43096

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants