Add `Non_Blocking_Scripts_Check` #519

swissspidy · 2024-07-11T14:59:12Z

This runtime check is pretty basic right now but does the following:

Test various URLs, similar to our other styles/scripts checks
Enqueues all scripts
Checks whether each enqueued script match the plugin (based on URL)
For matching scripts, checks whether they're enqueued in the footer or the header and if they have a loading strategy

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

github-actions · 2024-07-11T15:01:28Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: ernilambar <rabmalin@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

includes/Checker/Checks/Non_Blocking_Scripts_Check.php

adamsilverstein

Looks good! only some small feedback and a question.

includes/Checker/Checks/Non_Blocking_Scripts_Check.php

westonruter · 2024-07-11T16:27:13Z

includes/Checker/Checks/Non_Blocking_Scripts_Check.php

+				array(
+					'posts_per_page' => 1,
+					'post_type'      => $post_type,
+					'post_status'    => array( 'publish', 'inherit' ),


Why inherit?

Not sure, to be honest, I copied this from the other checks.

In PR #76, we added a first run time check. The b6e5e56 added the "inherit" post status. I don't think this check is necessary, and there has been no feedback or discussion on the PR regarding it. It appears to have been added by mistake, so we should remove it in a follow-up PR.

includes/Checker/Checks/Non_Blocking_Scripts_Check.php

Co-authored-by: Weston Ruter <westonruter@google.com>

swissspidy · 2024-07-12T09:28:01Z

Worth noting that this has some overlap with the WordPress.WP.EnqueuedResourceParameters.NotInFooter PHPCS sniff which we also run. But as per WordPress/WordPress-Coding-Standards#2420 it is limited and might even get removed.

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

mukeshpanchal27 · 2024-07-15T05:01:15Z

includes/Checker/Checks/Non_Blocking_Scripts_Check.php

+				array(
+					'posts_per_page' => 1,
+					'post_type'      => $post_type,
+					'post_status'    => array( 'publish', 'inherit' ),


In PR #76, we added a first run time check. The b6e5e56 added the "inherit" post status. I don't think this check is necessary, and there has been no feedback or discussion on the PR regarding it. It appears to have been added by mistake, so we should remove it in a follow-up PR.

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

mukeshpanchal27

Thanks for the PR! Overall looks good to me. Left some feedbacks.

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

mukeshpanchal27

Approved. Left one non-blocking feedback.

tests/phpunit/tests/Checker/Checks/Non_Blocking_Scripts_Check_Tests.php

…Tests.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

westonruter · 2024-07-16T23:18:15Z

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

+		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`.


Is this referring to before/after inline scripts? Or to scripts that are manually printed with wp_print_inline_script_tag() or otherwise?

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.

adamsilverstein · 2024-07-22T17:06:07Z

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

+			);
+
+			if ( ! isset( $posts[0] ) ) {
+				throw new Exception(


maybe just skip silently here, sicne a post type could have no posts?

This runtime check creates exactly 1 post in every post type, so it is guaranteed to have one.

adamsilverstein · 2024-07-22T17:07:17Z

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

+	protected function get_urls() {
+		$urls = array( home_url() );
+
+		foreach ( $this->get_viewable_post_types() as $post_type ) {


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

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.

adamsilverstein

overall looks good, left some small feedback

westonruter

A suggestion, but LGTM

westonruter · 2024-07-22T19:51:11Z

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

+		wp_scripts()->do_footer_items();
+		ob_end_clean();
+
+		foreach ( wp_scripts()->done as $handle ) {


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.

A new issue for this sounds like a good idea 👍

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

Co-authored-by: Weston Ruter <westonruter@google.com>

Add Non_Blocking_Scripts_Check

4c68736

swissspidy added [Type] Enhancement A suggestion for improvement of an existing feature Checks Audit/test of the particular part of the plugin [Team] Performance Issues owned by Performance Team labels Jul 11, 2024

Lint fix

30cedfe

Remove dead code

790f60e

swissspidy requested review from westonruter, joemcgill, ernilambar, adamsilverstein and mukeshpanchal27 July 11, 2024 15:06

adamsilverstein reviewed Jul 11, 2024

View reviewed changes

includes/Checker/Checks/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

adamsilverstein reviewed Jul 11, 2024

View reviewed changes

includes/Checker/Checks/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

adamsilverstein reviewed Jul 11, 2024

View reviewed changes

includes/Checker/Checks/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

adamsilverstein reviewed Jul 11, 2024

View reviewed changes

ernilambar reviewed Jul 11, 2024

View reviewed changes

includes/Checker/Checks/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

westonruter reviewed Jul 11, 2024

View reviewed changes

swissspidy and others added 5 commits July 12, 2024 11:09

Add missing entry

c665f51

Fix indentation

1f36417

Co-authored-by: Weston Ruter <westonruter@google.com>

Use ob_end_clean()

ddeaa8d

Co-authored-by: Weston Ruter <westonruter@google.com>

Adjust docblock

819d444

Adjust error message

7a74daf

Merge branch 'trunk' into fix/467-strategy

fa2eb87

swissspidy requested a review from felixarntz as a code owner July 13, 2024 12:26

Fix import

31fdb1d

mukeshpanchal27 reviewed Jul 15, 2024

View reviewed changes

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Jul 15, 2024

View reviewed changes

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Jul 15, 2024

View reviewed changes

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Jul 15, 2024

View reviewed changes

Apply suggestions from code review

295dc83

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

mukeshpanchal27 approved these changes Jul 16, 2024

View reviewed changes

tests/phpunit/tests/Checker/Checks/Non_Blocking_Scripts_Check_Tests.php Outdated Show resolved Hide resolved

Update tests/phpunit/tests/Checker/Checks/Non_Blocking_Scripts_Check_…

36e3bb3

…Tests.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

swissspidy added this to the 1.1.0 milestone Jul 16, 2024

westonruter reviewed Jul 16, 2024

View reviewed changes

Merge branch 'trunk' into fix/467-strategy

cdf5d58

swissspidy requested review from adamsilverstein, ernilambar and westonruter July 19, 2024 08:49

adamsilverstein reviewed Jul 22, 2024

View reviewed changes

westonruter approved these changes Jul 22, 2024

View reviewed changes

Apply suggestion to add script handle

b8c3b4e

Co-authored-by: Weston Ruter <westonruter@google.com>

swissspidy merged commit 436bcec into trunk Jul 23, 2024
21 checks passed

swissspidy deleted the fix/467-strategy branch July 23, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Non_Blocking_Scripts_Check` #519

Add `Non_Blocking_Scripts_Check` #519

swissspidy commented Jul 11, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

adamsilverstein left a comment

westonruter Jul 11, 2024

swissspidy Jul 11, 2024

mukeshpanchal27 Jul 15, 2024

swissspidy commented Jul 12, 2024

mukeshpanchal27 Jul 15, 2024

mukeshpanchal27 left a comment

mukeshpanchal27 left a comment

westonruter Jul 16, 2024

swissspidy Jul 17, 2024

adamsilverstein Jul 22, 2024

swissspidy Jul 22, 2024

adamsilverstein Jul 22, 2024

swissspidy Jul 22, 2024

adamsilverstein left a comment

westonruter left a comment

westonruter Jul 22, 2024

swissspidy Jul 22, 2024

Add Non_Blocking_Scripts_Check #519

Add Non_Blocking_Scripts_Check #519

Conversation

swissspidy commented Jul 11, 2024 • edited Loading

github-actions bot commented Jul 11, 2024 • edited Loading

adamsilverstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy commented Jul 12, 2024

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Non_Blocking_Scripts_Check` #519

Add `Non_Blocking_Scripts_Check` #519

swissspidy commented Jul 11, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading