Enhancing the WP_Scripts API With a Loading Strategy by 10upsimon · Pull Request #4391 · WordPress/wordpress-develop

10upsimon · 2023-04-27T22:19:57Z

Trac ticket: https://core.trac.wordpress.org/ticket/12009

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2023-04-27T22:20:16Z

Hi @10upsimon! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

felixarntz

I'll do a more detailed review soon, but just noting a few small things I spotted going over the code for early consideration.

src/wp-content/themes/twentytwentyone/functions.php

src/wp-includes/blocks.php

src/wp-includes/functions.wp-scripts.php

westonruter · 2023-04-27T22:57:27Z

When reviewing this PR, I was noticing how methods in WP_Scripts construct a lot of <script> tags manually with string concatenation, printf, variable interpolation, and calls to esc_attr()/esc_url(). Since WordPress 5.7, there have been dedicated helper functions (wp_get_inline_script_tag() and wp_get_script_tag()) which do all of the heavy lifting, as well as adding features like allowing the attributes to be filterable (for the sake of CSP). I've opened a draft PR (10up#58) for this branch which proposes using them. If desired, I can fix up the tests.

@adamsilverstein Do you know why these functions weren't utilized in WP_Scripts when they were introduced? I seem to recall this was punted for a later effort, but I can't find that info off hand.

src/wp-includes/class-wp-scripts.php

mukeshpanchal27

Thanks @10upsimon, Left some nit-pick feedback.

src/wp-includes/class-wp-scripts.php

src/wp-includes/functions.wp-scripts.php

tests/phpunit/tests/dependencies/scripts.php

…ng-strategy

…pendents

…ere is a defer dependency

…dling-redux Improve handling of delayed strategies when dependency aliases (bundles) are involved

…into feature/enhance-wp-scripts-api-with-a-loading-strategy * 'trunk' of https://github.com/WordPress/wordpress-develop: (30 commits) Themes: Inline render blocking CSS `classic-themes.css' Bundled Themes: Remove `load_theme_textdomain()` calls from default themes. I18N: Allow to short-circuit `load_textdomain()`. Coding Standards: Use strict comparison in `wp-includes/pomo/entry.php`. Themes: add wp_get_remote_theme_patterns function. Tests/Build tools: Various term related test improvements. Posts, Post Types: Introduce `item_trashed` post type label. Tests/Build tools: Add `@covers` annotation to `wp_set_object_terms()` tests. Taxonomy: Prevent deprecation notices clearing terms. Build/Test Tools: Revert accidental change to .env Media: Update admin image editor design. Docs: Use third-person singular verbs in various function descriptions, as per docblocks standards. Docs: Use third-person singular verbs in various function descriptions, as per docblocks standards. Docs: Use third-person singular verbs in various function descriptions, as per docblocks standards. Fix lint issues in WP_Theme_JSON::sanitize method. Coding Standards: Use strict comparison in `wp-includes/class-wp-oembed.php`. Twenty Seventeen: Improve Grid View variation rendering in the editor for the Post List block. Ignore unregistered block style variations from `theme.json`. Docs: Use third-person singular verbs in various function descriptions, as per docblocks standards. Editor: Skip file_exist check for core blocks. ...

westonruter

I feel good about the current state. But I'm biased.

spacedmonkey

Marking as ready to commit 🎉

azaozz · 2023-06-20T09:20:27Z

Marking as ready to commit

Yep, seems ready to commit except the code that re-adds support for "script after". I agree the discussion about adding that code can continue during beta, and if the decision is to continue to promote outdated, mediocre JS architecture it can be committed then.

However I'm against adding that in the initial commit. No matter how I look at it, it doesn't make sense to keep promoting this outdated method instead of asking the developers to transition to more modern JavaScript. This seems like a pretty bad "step backwards" from a code architecture point of view.

(As the code that adds support for "script after" is still in this PR, I'm not removing the "changes requested" label.)

This remove support for script handles with inline scripts attached to be loaded with either an `async` and `defer` strategy. Instead, they will be converted to a blocking strategy. This also consolidates logic for determining eligible loading strategies into a new private method, `WP_Scripts::filter_eligible_strategies`, which is responsible for recursively eliminating possible loading strategies. This removes duplication of logic that previously was in place for checking the eligible strategy of a handle with checking for whether dependents could be deferred. `WP_Scripts::has_only_deferrable_dependents` had been removed as it is now unnecessary. Unit tests are updated. Co-authored-by: Simon Dowdles <simon.dowdles@get10up.com>

joemcgill · 2023-06-22T15:50:47Z

@azaozz, I've removed the support for inline "after" scripts and would appreciate another look. What this means is that now any script handle that is registered with an async or defer loading strategy will be printed without either (i.e. will be loaded as a blocking script) if an inline "after" script is attached to it or any of the scripts depending on it.

spacedmonkey

Approved again.

felixarntz

@10upsimon @joemcgill Looks great! A few minor nit-picks, but none of them strictly a blocker.

src/wp-includes/class-wp-scripts.php

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

spacedmonkey · 2023-06-26T13:42:09Z

Committed https://core.trac.wordpress.org/changeset/56033

azaozz · 2023-06-26T19:53:28Z

I've removed the support for inline "after" scripts and would appreciate another look.

Sorry for the delay, was afk dealing with some family stuff. The commit LGTM, thanks!

thefrosty · 2023-06-27T15:28:37Z

For those whom might be using PHP >= 8.0's named arguments, this will break in WordPress 6.3.0.
!
named argument example method

Removing the in_footer prop will result in a "Cannot use a positional argument after a named argument" error.

I would suggest implementing a stub in your code until you've had time to resolve potential issues.

Just calling it out for those who might come looking.

felixarntz requested review from felixarntz, joemcgill and westonruter April 27, 2023 22:21

This was referenced Apr 27, 2023

DRAFT: PR for enhancing WP_Scripts with a loading strategy 10up/wordpress-develop#54

Closed

Eliminate manual construction of script tags 10up/wordpress-develop#58

Closed

felixarntz reviewed Apr 27, 2023

View reviewed changes

westonruter requested changes Apr 28, 2023

View reviewed changes

mukeshpanchal27 reviewed Apr 28, 2023

View reviewed changes