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

Enhance detection of enqueued frontend assets #136

Conversation

manuelRod
Copy link
Contributor

@manuelRod manuelRod commented Jan 28, 2022

Summary

Enhances for Site Health Enqueued assets module.
Fixes #135

Relevant technical choices

Follow up of the previous closed PR #54

Checklist

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

Sorry, something went wrong.

@manuelRod manuelRod requested a review from audrasjb as a code owner January 28, 2022 11:59
@manuelRod manuelRod marked this pull request as draft January 28, 2022 11:59
@eclarke1 eclarke1 added [Focus] Site Health [Type] Feature A new feature within an existing module labels Jan 28, 2022
@felixarntz felixarntz added this to the 1.0.0-beta.1 milestone Jan 28, 2022
@felixarntz felixarntz changed the title Add/sitehealth enqueued assets enhance Enhance detection of enqueued frontend assets Jan 28, 2022
@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature and removed [Type] Feature A new feature within an existing module labels Jan 28, 2022
@felixarntz felixarntz marked this pull request as ready for review February 18, 2022 16:09
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@manuelRod This looks solid to me!

Would love to get someone else to review as well, can you maybe give this a pass @aristath?

Comment on lines 34 to 35
$inline_size = 0;
if ( ! empty( $script->extra ) && ! empty( $script->extra['after'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the above.

Suggested change
$inline_size = 0;
if ( ! empty( $script->extra ) && ! empty( $script->extra['after'] ) ) {
if ( empty( $script->extra['after'] ) || ! is_array( $script->extra['after'] ) ) {
continue;
}
$inline_size = 0;

Copy link
Contributor Author

@manuelRod manuelRod Mar 4, 2022

Choose a reason for hiding this comment

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

this would break functionality. If empty extra['after'] $inline_size will be 0 and that's it, we don't need to skip the iteration with continue, since the loop is not finished.

Comment on lines 69 to 73
if ( ! empty( $style->extra ) && ! empty( $style->extra['path'] ) ) {
$path = $style->extra['path'];
} else { // Fallback to getting the path from the style's src.
$path = perflab_aea_get_path_from_resource_url( $style->src );
}
Copy link
Member

@mitogh mitogh Feb 28, 2022

Choose a reason for hiding this comment

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

By inverting the conditional here allows the removal of the ! operator, allowing for a more natural flow.

Suggested change
if ( ! empty( $style->extra ) && ! empty( $style->extra['path'] ) ) {
$path = $style->extra['path'];
} else { // Fallback to getting the path from the style's src.
$path = perflab_aea_get_path_from_resource_url( $style->src );
}
// default to getting the path from the style's src.
if ( empty( $style->extra ) || empty( $style->extra['path'] ) ) {
$path = perflab_aea_get_path_from_resource_url( $style->src );
} else {
$path = $style->extra['path'];
}

Copy link
Member

Choose a reason for hiding this comment

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

@mitogh Not sure I agree in this case, the original code here explicitly looks at whether the $style->extra value is not empty, and the other function use would be the fallback. I don't have a strong preference on the order, but with your suggestion here the comment in line 71 is no longer accurate.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right let me update line 71.

Mostly this pattern avoids the usage of ! which translates 1 operation into 2.

Copy link
Member

Choose a reason for hiding this comment

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

I'd argue that's a bit of a micro optimization. IMO the code path checking ! empty( $var ) and then using $var is more straightforward to read.

Copy link
Member

Choose a reason for hiding this comment

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

The suggestion goes more towards optimization for reading rather than performance so it becomes a personal preference I would say either way is fine, but IMO this seems easier to read:

  • If empty then
  • Else

Rather than

  • If not empty then
  • else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't agree with this (since is a personal preference), to me, seems more natural and easier to read the way it is right now.


// Add any extra data (inlined) that was passed with the style.
$inline_size = 0;
if ( ! empty( $style->extra ) && ! empty( $style->extra['after'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to the above.

Copy link
Contributor Author

@manuelRod manuelRod Mar 4, 2022

Choose a reason for hiding this comment

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

same as above, it would break functionality.

Copy link
Contributor

@dainemawer dainemawer left a comment

Choose a reason for hiding this comment

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

Nice work overall, left a few questions!

@@ -78,14 +78,29 @@ function perflab_aea_get_total_size_bytes_enqueued_styles() {

/**
* Convert full URL paths to absolute paths.
* Covers Standard WP configuration, wp-content outside WP directories and subdirectories.
* Ex: https://example.com/content/themes/, https://example.com/wp/wp-includes/
*
* @since 1.0.0
*
* @param string $resource_url URl resource link.
* @return string Returns absolute path to the resource.
*/
function perflab_aea_get_path_from_resource_url( $resource_url ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Im noticing quite a few varying function namespaces - maybe this function should be called perflab_get_path_from_resource_url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello, @dainemawer all functions are prepending "perflab_aea", what are the function namespaces varying? maybe I'm missing some

Copy link
Member

Choose a reason for hiding this comment

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

I think the approach of having module specific namespaces (all of them starting with perflab_ though due to the overall plugin) is a solid approach, which avoids us running into accidental conflicts between individual modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

@manuelRod I was thinking more globally across the plugin - I've noticed other functions are all prefixed with perflab_ but not necessarily perflab_aea - that being said it sounds like @felixarntz is good with this approach - Im not swayed either way - just wanted to call it out for consistency reasons :)

'src' => $src,
'size' => perflab_aea_get_resource_file_size( perflab_aea_get_path_from_resource_url( $src ) ),
'src' => $script->src,
'size' => perflab_aea_get_resource_file_size( $path ) + $inline_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this size take into account the size on the server / disk - or the transferred / gzipped size? Probably worth noting as the 2 can be different and could skew results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the disk/server size. It uses filesize() to get the file size.

Copy link
Contributor

Choose a reason for hiding this comment

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

@manuelRod - that makes sense - just so Im understanding correctly, would that size be more or less the same than transferred / gzipped size?


/**
* Audit enqueued styles in the frontend. Ignore /wp-includes styles.
* Audit enqueued and printed styles in the frontend. Ignore /wp-includes styles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we ignore wp-includes? Gutenberg for instance adds a couple of stylesheets that arent exactly optimized. My opinion is that we should give the user the full picture, even if it means flagging WP for enqueuing unnecessary stylesheets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dainemawer those assets are included by default by wp, and this module focus on an overall overview of external assets being enqueued (plugins, theme), I think it would make more noise to include wp-includes, since most of the consumers won't be techies and won't know how to react on it.
I agree that going forward and as we iterate over this module, we can show more useful information, at that point, we can add more info (like wp-includes) and tips on how to improve it, but for now, I would just ignore them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats a fair statement, thanks @manuelRod !

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

Apologies it took me so long to get to this PR.
The code looks good to me and makes sense 👍

@felixarntz felixarntz changed the base branch from trunk to release/1.0.0-beta.1 March 3, 2022 00:45
@felixarntz
Copy link
Member

@manuelRod FYI Since we've branched off from trunk into a release/1.0.0-beta.1 branch, I've updated the base branch of this PR to that new branch. So going forward, please make sure to work off release/1.0.0-beta.1, and specifically not merge trunk into this again.

@manuelRod
Copy link
Contributor Author

applied some of the comments! not sure if anyone wants to review them again to be ready for the release @felixarntz @aristath

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@manuelRod Two more tiny changes needed here based on the change from https://github.com/WordPress/performance/pull/136/files#r819490106, I can quickly add them.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Added the two tiny changes, all the rest already lgtm before. Thank you, great work @manuelRod!

@felixarntz felixarntz merged commit 675bca2 into WordPress:release/1.0.0-beta.1 Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance reliability of how Site Health enqueued assets module detects assets
6 participants