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

chore(perf): include a high-priority image in Link response headers, if available #14974

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

joeyAghion
Copy link
Contributor

(Work-in-progress for feedback only.)

This is heavily based on #14827, and a few naïve assumptions. It attempts to add a Link: header on responses indicating that browsers should preload an image from the rendered HTML.

  • It only covers the first image with fetchpriority="high".
  • It uses only the src attribute of that img element, so doesn't benefit from srcSet and may even cause a 2x browser to preload a 1x image unnecessarily.

Copy link

relativeci bot commented Dec 11, 2024

#1314 Bundle Size — 8.95MiB (-0.03%).

14b11e7(current) vs ce82e30 main#1313(baseline)

Warning

Bundle contains 14 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Improvement 1 improvement
                 Current
#1314
     Baseline
#1313
Improvement  Initial JS 3.65MiB(~-0.01%) 3.65MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 41.34% 39.38%
No change  Chunks 103 103
No change  Assets 106 106
Change  Modules 5834(-0.03%) 5836
No change  Duplicate Modules 530 530
No change  Duplicate Code 4.03% 4.03%
No change  Packages 266 266
No change  Duplicate Packages 13 13
Bundle size by type  Change 1 change Improvement 1 improvement
                 Current
#1314
     Baseline
#1313
Improvement  JS 8.81MiB (-0.03%) 8.81MiB
No change  Other 143.36KiB 143.36KiB

Bundle analysis reportBranch joeyAghion/lcpHintProject dashboard


Generated by RelativeCIDocumentationReport issue

@damassi
Copy link
Member

damassi commented Dec 12, 2024

This was something that I noticed in the first version, where we'd be overfetching images. Check out this artwork page:

PR (three images):

Screenshot 2024-12-11 at 4 06 57 PM

Staging (two images):

Screenshot 2024-12-11 at 4 07 25 PM

Might be worth removing the <link> tag from that page and then trying to hint it using the automated process in here (but that said, we might need to also set the imageSrcSet for this to be valuable for our demographic (who are typically on 2x devices).

@joeyAghion
Copy link
Contributor Author

This PR currently results in Link: headers as follows:

  • for artwork pages, the quality=1 loading placeholder image, which I think is correct (if sneaky)
  • for artist pages, the lower-resolution of the 2 available main images, which seems reasonable (even if we could sense whether the client is 1x- or 2x-capable, Cloudflare would cache and return that hint to other clients)

I will try to verify if this actually improves LCP locally or in a review app.

Questions:

  • In both cases, it only returns a single image. Was your example referring to the prior PR?
  • Alternatively, what if we searched in the body for <link rel="preload"... instead of <img fetchpriority="high"... as this does now? It might be more explicit.

@damassi
Copy link
Member

damassi commented Dec 12, 2024

Discussed this on slack. But yah, makes sense! We should just prioritize the 2x images getting into the header tags and then 🚢

@joeyAghion
Copy link
Contributor Author

joeyAghion commented Dec 16, 2024

Latest commit avoids any sort of HTML-parsing and just extracts the images for preloading from the already available head tags. That way, any components can render <link> tags via react-head, and they'll automatically appear in response headers. I think this is much cleaner than parsing the body for <img ...fetchpriority="high"> tags.

With this code, artwork pages respond with 2 images for preloading (the quality=1 placeholder and the main image that replaces that), while artist pages respond with the single header image, as expected.

Next question, which can be considered in this PR or a separate one: those pages currently populate link tags' hrefs with the "1x" image variant, similar to the img tags' src attribute. If we believe the "2x" variant will be more commonly needed by browsers, maybe we should default to the "2x" version in those href/src attributes. (That way, these response headers will automatically preload that.)

damassi
damassi previously approved these changes Dec 16, 2024
Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

Love this elegant solution 👍

Lets hint the 2x image and monitor for now, but otherwise merge when ready!

@joeyAghion joeyAghion merged commit 413968e into main Dec 17, 2024
12 of 13 checks passed
@joeyAghion joeyAghion deleted the joeyAghion/lcpHint branch December 17, 2024 17:59
@artsy-peril artsy-peril bot mentioned this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants