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

Preloading images and stylesheets: <link> vs Early Hints response header #617

Draft
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

felixarntz
Copy link
Member

Summary

This is an experiment. Do not merge.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@felixarntz felixarntz added [Type] Feature A new feature within an existing module [Focus] Images Issues related to the Images focus area [Focus] JS & CSS Issues related to the JS & CSS focus area (formerly JavaScript) no milestone PRs that do not have a defined milestone for release labels Dec 26, 2022
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @felixarntz Left some feedback.

Comment on lines +40 to +45
if ( null !== $ver ) {
if ( false === $ver ) {
$ver = wp_styles()->default_version;
}
$preload = add_query_arg( 'ver', $ver, $preload );
}
Copy link
Member

Choose a reason for hiding this comment

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

When the version of the stylesheet is not define then we didn't get theme version. ex. wp_enqueue_style( 'style', get_stylesheet_uri(), array(), '' );

* @since n.e.x.t
*/
function perflab_tseh_reset_stylesheet_check() {
delete_option( 'perflab_preload_stylesheet' );
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure to delete the new option in uninstall.php.

Comment on lines +17 to +20
// Bail if not a frontend request.
if ( is_admin() || defined( 'XMLRPC_REQUEST' ) || defined( 'REST_REQUEST' ) || defined( 'MS_FILES_REQUEST' ) ) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Similar conditions are checked in perflab_tseh_send_early_hints_header(). Add a new function and use it.

}
);
}
add_action( 'plugins_loaded', 'perflab_hieh_send_early_hints_header' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need all this logic if we can just check the current post thumbnail ID and preload that image if it is available? If there is no thumbnail for the post, then I think we should preload nothing because the first image in the content is not always the hero image. Furthermore, the first image in the content can be far below the fold and in this case preloading it is inefficient. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@eugene-manuilov What you're saying about the first content image can certainly happen, though at the same time it even applies to the featured image, it entirely depends on the theme where that is displayed. It usually is in the header area but not always.

Each of these concerns already apply to the fetchpriority="high" and loading="lazy" features too to be fair.

@felixarntz
Copy link
Member Author

@mukeshpanchal27 @eugene-manuilov Thank you for the feedback. Just to highlight, this PR is not meant to be merged. It's fine to review it, but there's a couple things in here that I just quickly hacked together and that would certainly not be a proper approach if we wanted to truly make this a module. Just as some context; I'll probably eventually close this, not really worth too much polishing (unless something is broken that even affects the lab tests we're running with this code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Images Issues related to the Images focus area [Focus] JS & CSS Issues related to the JS & CSS focus area (formerly JavaScript) no milestone PRs that do not have a defined milestone for release [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants