Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#519Add
Non_Blocking_Scripts_Check
#519Changes from 10 commits
4c68736
30cedfe
790f60e
c665f51
1f36417
ddeaa8d
819d444
7a74daf
fa2eb87
31fdb1d
295dc83
36e3bb3
cdf5d58
b8c3b4e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 69 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Codecov / codecov/patch
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L69
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.
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.
Check warning on line 143 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Codecov / codecov/patch
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L142-L143
Check warning on line 147 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Codecov / codecov/patch
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L145-L147
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 thescript_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 👍
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.
Check warning on line 188 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
Codecov / codecov/patch
includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L188