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

Improve performance capabilities of core script enqueueing #168

Closed
adamsilverstein opened this issue Feb 11, 2022 · 10 comments
Closed

Improve performance capabilities of core script enqueueing #168

adamsilverstein opened this issue Feb 11, 2022 · 10 comments
Labels
Needs Discussion Anything that needs a discussion/agreement [Type] Epic A high-level project / epic that will encompass several sub-issues WP Core Work relates to inclusion in WP Core only

Comments

@adamsilverstein
Copy link
Member

Feature Description

Goal:
Add backwards compatible script enqueue features to WordPress that enable more performant script enqueueing, including support for defer.

Details
WordPress core, themes and plugins add script tags to the page through a number of mechanisms, for example wp_enqueue_script, wp_print_script_tag or by directly outputting their script in a wp_head action handler.

This Issue focuses primarily on wp_enqueue_script (the recommended method for adding scripts in WordPress) which currently only lets developers control their script’s dependencies (other previously defined scripts) and whether the script should be enqueued in the header or footer (ref).

Developers can control what order their own scripts are output (the order they are enqueued), but not how they are prioritised in relation to scripts enqueued by other parts of the system (eg other plugins). In addition, wp_enqueue_script does not currently directly support adding attributes like “defer” to script tags, although wp_print_script_tag which was added in version 5.7 does support script tag attributes (ticket).

Proposal

Step 1. Add a loading strategy

  • Add loading strategy support to wp_enqueue_script function (secondarily admin_enqueue_script)
    • Follow up with work on https://core.trac.wordpress.org/ticket/22249
    • strategy could be a new parameter or we could overload "in_footer": using a string for strategy, true for footer and null or false for header.
    • For strategy I propose “block” (equivalent to today’s head scripts) or “defer” (equivalent to today’s footer scripts, but deferred), open to naming suggestions.
  • Automatic loading optimizations
    • Footer grouping - when a script has in_footer set to true, dependent/dependency scripts should also be moved to the footer
    • When a script uses strategy defer, dependent/dependency scripts should also use defer
    • Scripts added with wp_add_inline_script hooked on a deferred script should also be deferred.
  • Enable opt out of the new behavior with a single filter (eg add_filter( ‘wp_optimize_script_loading’, ‘__return_false’ );)

Step 2. Consume new API in core

  • Audit existing front end enqueued scripts for defer strategy opportunities (eg. ‘comments’)

Step 3. Future API enhancements

  • Consideration for wp_print_script_tag, wp_get_inline_script_tag, wp_print_inline_script_tag and wp_get_script_tag or even directly injected scripts and how these might be improved with a strategy, or to encourage/enablel defer.
  • Having a strategy attribute is forward compatible, letting us consider additional loading strategies in the future:
    • “priority” would use priority hints (or preload plus delayed js injection) to load the script with a high priority in a non blocking way
    • “worker” would put a script in a worker using a tool like PartyTown, possibly useful for third party scripts or scripts that don't need much/any DOM access

Related


I welcome any feedback here about the proposed approach here, then we could split this out into several sub issues for the steps described above.

@adamsilverstein adamsilverstein added [Focus] JavaScript [Issue] Overview Provides an overview of a specific project Needs Discussion Anything that needs a discussion/agreement labels Feb 11, 2022
@adamsilverstein adamsilverstein added this to Backlog in [Focus] JS & CSS via automation Feb 11, 2022
@adamsilverstein adamsilverstein moved this from Backlog to To do in [Focus] JS & CSS Feb 11, 2022
@eclarke1 eclarke1 added [Type] Epic A high-level project / epic that will encompass several sub-issues and removed [Issue] Overview Provides an overview of a specific project labels Feb 15, 2022
@eugene-manuilov
Copy link
Contributor

  • When a script uses strategy defer, dependent/dependency scripts should also use defer

Where will the deferred script be loaded? In the footer or in the header? If in the footer, then how can I load it in the header if I need to?

Maybe we should leave the current wp_enqueue_script function as is and add a helper function to defer scripts? Something like wp_defer_script( $hanlder )?

wp_enqueue_script( 'header-script', '.../header.js', array( '...' ), '1.0', false );
wp_enqueue_script( 'footer-script', '.../footer.js', array( '...' ), '1.0', true );

wp_defer_script( 'header-script' );

Also, does it really make sense to defer scripts loaded in the footer? Correct me if I am wrong, but the idea of deferred scripts is in the fact that they are loaded asynchronously and executed once the document parsing is over which usually happens right after loading footer scripts.

@mitogh
Copy link
Member

mitogh commented Feb 15, 2022

Add loading strategy support to wp_enqueue_script function (secondarily admin_enqueue_script)

For backward compatibility introduce a new array parameter with attributes to allow specific additional attributes such as defer or async as If this is done in a way that works only for defer having a more flexible approach would be great to support additional attributes.

@westonruter
Copy link
Member

wp_defer_script( 'header-script' );

Another alternative using an existing function:

wp_script_add_data( 'header-script', 'strategy', 'defer' );

@jonoalderson
Copy link

jonoalderson commented Feb 16, 2022

Relying on additional function (like wp_script_add_data) will harm adoption; and in fact, already has. This requires people to actively know that such a function exists, understand it, and use it. That's far more complex than, e.g., just picking a suitable value when confronted with needing to select a strategy.

@adamsilverstein
Copy link
Member Author

reading up on inline scripts that appear after a script that uses the defer strategy, we should able to handle those with type="module" which will cause them to be deferred as well, thus continuing to be evaluated after the deferred script.

@Kevin-Hamilton
Copy link

You can't take an existing script block of unknown code and put a type="module" on it and expect it to "just work". For instance,

  • modules use strict mode automatically,
  • HTML comments are not allowed in modules,
  • the this within modules does not refer to the global this, and instead is undefined,
  • await cannot be used as a variable name anywhere in a module, and
  • variables defined in a module are not available in the global scope unless explicitly attached to the window object.

Also, (and I apologize if this is the wrong venue to bring this up) I would like to make the suggestion that any efforts to get an async and/or defer attribute incorporated into the WP script enqueuing APIs should take a back-seat priority to getting support for the nonce attribute within the WP script enqueuing APIs. If both could get done together, that would be fine but the nonce attribute doesn't even seem to be on the agenda here and defer introduces some systemic problems with respect to inline dependencies and dependents which to my mind should decouple and de-prioritize it from generalized and very necessary API changes needed for simpler attributes like the CSP nonce attribute.

@adamsilverstein
Copy link
Member Author

should take a back-seat priority to getting support for the nonce attribute within the WP script enqueuing APIs

@Kevin-Hamilton thanks for raising this, is there an existing ticket about the CSP nonce attribute? I know there were numerous work streams for CSP that have stalled so I assume those cover this. This is a security measure, correct?

You can't take an existing script block of unknown code and put a type="module" on it and expect it to "just work". For instance,

Good points to raise and perhaps indicates we can't automate the deferal of scripts with inlined dependents. The goal isn't to automate defer entirely but rather to provide a 1p API for defer that helps work thru some of the dependency issues.

@Kevin-Hamilton
Copy link

@adamsilverstein - regarding CSP nonce, yes it is a security measure. For a deeper dive on the topic, I recommend this video: https://www.youtube.com/watch?v=_L06HetskC4.

Regarding existing tickets - I believe you were owner of the ticket that resulted in the 4 new API functions added to WP 5.7: https://make.wordpress.org/core/2021/02/23/introducing-script-attributes-related-functions-in-wordpress-5-7/. Unfortunately, these are woefully inadequate as they don't do anything for scripts that are part of the enqueue APIs. For example, wp_add_inline_script and wp_localize_script will both output script tags to the browser without any filters available for the security-conscious programmer to intercept that output and modify the attributes of the script tag. This is a big problem problem because those functions are very widely used. See: https://wpdirectory.net/search/01GDC5GQSKYCVK6SE517N97E5E and https://wpdirectory.net/search/01GBM897NXNDXBYVQSNVNW0B22. Compare that to the only 371 instances of those new API functions being implemented so far in the last 18 months since WP 5.7 was released: https://wpdirectory.net/search/01GBM7K6GGNRE5AA2BG8VPVHQP.

Here is one open ticket on this issue: https://core.trac.wordpress.org/ticket/54214 but it doesn't cover the wp_localize_script use case. I guess there could have been other tickets in recent years that have been closed as duplicates of https://core.trac.wordpress.org/ticket/12009 or https://core.trac.wordpress.org/ticket/22249. I fear that focusing on the generic solution that can cover all script attributes including async and defer has come at an opportunity cost for particular attributes like nonce which do not have the edge cases that defer has.

Meanwhile, there are plugins like this one which people might install to become "CSP compliant", but is actually useless for protecting against a stored or reflected XSS attack because it is just using a regex against buffered output to put the nonce on every tag, ignorant of whether that script was added by core/plugin/theme, or by attacker!

I apologize for dumping all this information at your feet, and I know the core performance team is not the appropriate place for the discussion, but to my surprise there doesn't even seem to be a dedicated core security team at make.wordpress.org: https://make.wordpress.org/, nor a dedicated security channel in Slack.

@adamsilverstein
Copy link
Member Author

@Kevin-Hamilton I agree we should word to land nonce support in wp_enqueue_script, I don't see that covered in the existing tickets but maybe I missed it. The new functions were added as a way to avoid introducing any backwards compatibility breakage. One idea to cover both a nonce and a loading strategy would be to add a new $args array parameter that included both.

@felixarntz felixarntz added the WP Core Work relates to inclusion in WP Core only label Jul 19, 2023
@felixarntz
Copy link
Member

This has been implemented for WordPress 6.3 in https://core.trac.wordpress.org/ticket/12009, hence marking this as completed. 🎉

Also see #755.

[Focus] JS & CSS automation moved this from To do to Done Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion Anything that needs a discussion/agreement [Type] Epic A high-level project / epic that will encompass several sub-issues WP Core Work relates to inclusion in WP Core only
Projects
No open projects
Development

No branches or pull requests

8 participants