-
Notifications
You must be signed in to change notification settings - Fork 43
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 Non_Blocking_Scripts_Check
#519
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
Looks good! only some small feedback and a question.
array( | ||
'posts_per_page' => 1, | ||
'post_type' => $post_type, | ||
'post_status' => array( 'publish', 'inherit' ), |
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.
Why inherit?
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, to be honest, I copied this from the other checks.
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.
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Worth noting that this has some overlap with the |
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Outdated
Show resolved
Hide resolved
array( | ||
'posts_per_page' => 1, | ||
'post_type' => $post_type, | ||
'post_status' => array( 'publish', 'inherit' ), |
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.
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Outdated
Show resolved
Hide resolved
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Outdated
Show resolved
Hide resolved
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Outdated
Show resolved
Hide resolved
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.
Thanks for the PR! Overall looks good to me. Left some feedbacks.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
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.
Approved. Left one non-blocking feedback.
tests/phpunit/tests/Checker/Checks/Non_Blocking_Scripts_Check_Tests.php
Outdated
Show resolved
Hide resolved
…Tests.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
foreach ( wp_scripts()->done as $handle ) { | ||
$script = wp_scripts()->registered[ $handle ]; | ||
|
||
// TODO: Somehow detect inline scripts added by the plugin that don't have a `src`. |
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.
Is this referring to before/after inline scripts? Or to scripts that are manually printed with wp_print_inline_script_tag()
or otherwise?
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.
Both I suppose :-)
Right now the detection whether a script is coming from a plugin is very basic. Something like Origination + loopback requests would be more robust.
); | ||
|
||
if ( ! isset( $posts[0] ) ) { | ||
throw new Exception( |
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.
maybe just skip silently here, sicne a post type could have no posts?
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 runtime check creates exactly 1 post in every post type, so it is guaranteed to have one.
protected function get_urls() { | ||
$urls = array( home_url() ); | ||
|
||
foreach ( $this->get_viewable_post_types() as $post_type ) { |
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.
maybe add a sanity limit here, check at most 5 or 10 post types, I can imagine some site has 10,000 post types and this breaks site health for them
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.
Hmm no other plugins are active during checks and we're checking only a specific plugin here, I doubt that a single plugin adds that many post types.
If we add a limit, then we'd need to add it to all the runtime checks, but I don't see a need for it.
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.
overall looks good, left some small feedback
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.
A suggestion, but LGTM
wp_scripts()->do_footer_items(); | ||
ob_end_clean(); | ||
|
||
foreach ( wp_scripts()->done as $handle ) { |
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 just realized what may be a more robust way to handle this, which will also include support for inline scripts: use HTML Tag Processor to iterate over generated markup instead of iterating over the done
handles. There could be one output buffer for the head and another for the footer. This would then handle cases where a plugin is adding the strategy attribute manually via the script_loader_tag
filter. It would also flag cases where a script fails to get a strategy due to a blocking dependent script depending on an async/defer script.
But that can be done 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.
A new issue for this sounds like a good idea 👍
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Weston Ruter <westonruter@google.com>
This runtime check is pretty basic right now but does the following:
Warnings are reported if either is the case:
a) The script is enqueued in the header, without async/defer -> suggest adding async/defer
b) The script is enqueued in the footer -> suggest adding async/defer
Fixes #467