-
Notifications
You must be signed in to change notification settings - Fork 85
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 initial detection logic for Image Loading Optimization #876
Add initial detection logic for Image Loading Optimization #876
Conversation
} ); | ||
} | ||
|
||
// TODO: Use a local copy of web-vitals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you'd like help with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This todo isn't to be done in this PR. I'm working on this in #878.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I thought you commented on a different todo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was presuming that there would be a JS build process introduced to the plugin as part of #864 and so we could incorporate the use of a local copy of web-vitals as part of that. But actually it looks like it's not going to involve JS. If you think a JS build process could be incorporated as part of the PR, I'd appreciate your help. But otherwise I think this can be tackled in a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonruter Just looking through these changes now, I think it would be good to open a separate issue/PR for introducing a build process for JS. This can be against the feature branch because no other module needs it so far, and I don't anticipate any new module being introduced in the near future where it would be needed. So we can do that as part of this one. Let's just make sure the approach could be used by other modules too.
// TODO: Send data to server. | ||
log( pageMetrics ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to be done in another PR. See #878.
b73f8e8
to
73dbfd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions, but generally this looks like a good start. No reason to block this for the feature branch.
.eslintrc.js
Outdated
@@ -8,7 +8,15 @@ const config = { | |||
rules: { | |||
...( wpConfig?.rules || {} ), | |||
'jsdoc/valid-types': 'off', | |||
'no-console': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's a good idea to turn this off for the whole plugin. Perhaps we can just disable it where a console.log
is expected instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 912abef
const pageMetrics = { | ||
viewport: { | ||
width: win.innerWidth, | ||
height: win.innerHeight, | ||
}, | ||
elements: [], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious how you're thinking of using various viewport dimensions in the data when applying optimizations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it turns out that that there are different LCP images for different viewports, then fetchpriority
would not be added to the img
elements themselves. Instead the viewport
widths captured here would then be used to construct the media
attribute of preload
links so that only the relevant image URL is prioritized for the given viewport. In this way, the same markup can be served to both desktop and mobile, while both still get optimized.
a3e8c10
into
feature/image-loading-optimization
* Allow this amount of milliseconds between when the page was first generated (and perhaps cached) and when the | ||
* detect function on the page is allowed to perform its detection logic and submit the request to store the results. | ||
* This avoids situations in which there is missing detection metrics in which case a site with page caching which | ||
* also has a lot of traffic could result in a cache stampede. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Summary
Add logic to detect the LCP element and images and elements with background images which are in the initial viewport.
See #870.
This is currently a sub-PR of #875.
Relevant technical choices
The logic is contained in a
detect.js
script which gets imported as a module from an inline module script. This module script is currently printed to the page with every page load, but once storage (#871) is implemented, it will be conditionally added based on whether no existing metrics are available.The logic in
detect.js
only runs if it has been invoked within 5 seconds of the page being generated. This ensures that if the page output is cached, any future conditional serving of the detection script will not end up resulting in many clients running the logic unnecessarily when page caching is involved. The 5-second window can be filtered byperflab_image_loading_detection_time_window
.The detection logic involves gathering up a list of the optimizable elements, namely images and elements with background images (for now). For each of these elements, breadcrumbs are obtained which is an array of objects containing the tag name and element index. For example, the custom logo's breadcrumbs are:
These breadcrumbs are captured as soon as the module runs so that any subsequent dynamic insertions of new elements by JavaScript won't invalidate the breadcrumbs to look up the elements on the server-rendered page at the end of output buffering.
With the list of optimizable elements in hand, an IntersectionObserver is used to keep track of when they become visible (if ever). If the initial page load is not at the top of the page then the detection logic aborts since we don't want to capture metrics for an anchor link. Similarly, the IntersectionObserver disconnects as soon as the user starts scrolling. Elements contained inside of the admin bar are ignored.
In addition to observing for visible optimizable elements, the
onLCP
function from web-vitals.js library is used to obtain the LCP element metric. CurrentlyonLCP
is invoked withreportAllChanges
astrue
as otherwise it will only fire when the user clicks or presses a key. So at the very least for development purposes,reportAllChanges
is enabled. When the window'sload
event fires, then the latest LCP candidate metric reported is considered LCP.Finally, page metrics are gathered to send to the server, including:
For example:
With these metrics in hand, the server can to store them (#871) and make use of them to optimize the loading of those elements on the page (#872).
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.