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

Add web worker offloading plugin #1247

Merged
merged 108 commits into from
Aug 6, 2024
Merged

Add web worker offloading plugin #1247

merged 108 commits into from
Aug 6, 2024

Conversation

thelovekesh
Copy link
Member

@thelovekesh thelovekesh commented May 24, 2024

Summary

Fixes #176

Previously: #556

Relevant technical choices

  • Add Web Worker Offloading standalone plugin which allows running scripts within the worker thread with the help of the Partytown library.
  • Add the wwo_configuration filter which can be used to update web worker configurations.

Testing steps

  • Enable this performance plugin or use wp-env in which this plugin is enabled by default on this branch.

  • Add a JS script with web-worker-offloading as a dependency. When the page is loaded, the JS script should be loaded within the web worker.

    function enqueue_non_critical_js() {
        wp_enqueue_script(
      	  'non-critical-js',
      	  '/path/to/non-critical.js',
      	  array( 'web-worker-offloading' ), // Note the `web-worker-offloading` being added as a dep
      	  '1.0.0',
      	  true
        );
    }
    add_action( 'wp_enqueue_scripts', 'enqueue_non_critical_js' );
  • Use wwo_configuration hook to update web worker configurations.

    function update_wwo_config( $config ) {
      $config["debug"] = true;
      return $config;
    }
    add_filter( 'wwo_configuration', 'update_wwo_config' );
@thelovekesh thelovekesh added no milestone PRs that do not have a defined milestone for release [Type] Plugin Proposal A new plugin proposal labels May 24, 2024
@thelovekesh
Copy link
Member Author

@westonruter @adamsilverstein I have converted this into a standalone plugin. Next, I'll add a settings page to opt-in offloading for scripts listed in the Partytown documentation (https://partytown.builder.io/common-services). This way, the WordPress plugin team won't classify it as a "framework" plugin, as @westonruter previously mentioned.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some trivial suggestions left from me. Pre-approving so that once @adamsilverstein approves this can finally be merged!

thelovekesh and others added 2 commits August 2, 2024 14:30
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
@adamsilverstein adamsilverstein self-requested a review August 5, 2024 15:36
array_key_exists( $handle, wp_scripts()->registered ) &&
in_array( 'web-worker-offloading', wp_scripts()->registered[ $handle ]->deps, true )
) {
$html_processor = new WP_HTML_Tag_Processor( $tag );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@adamsilverstein
Copy link
Member

Code looks good to me, doing some manual testing next.

@adamsilverstein
Copy link
Member

Testing this locally with the sample code from the ticket description, I noticed the script handle is different, I had to use
'web-worker-offloading' while the instructions for testing in the description use 'web-worker-offloader'. Can you update that in the testing instructions for posterity?

Once I did that, the tag got the partytown type added:

image

question: do we want to remove the data-wp-strategy attribute as well?

I didn't test actual Partytown functionality yet, that will be easier to do once we ship the plugin. Are we planning add script specific handling (eg analytics) in the future?

@adamsilverstein adamsilverstein self-requested a review August 5, 2024 18:48
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tremendous!

@westonruter
Copy link
Member

Testing this locally with the sample code from the ticket description, I noticed the script handle is different, I had to use
'web-worker-offloading' while the instructions for testing in the description use 'web-worker-offloader'. Can you update that in the testing instructions for posterity?

Ah, that's my bad. I updated the slug in the codebase but didn't do it in the description. Updated now.

question: do we want to remove the data-wp-strategy attribute as well?

You're right. This can be removed as well. Or the async attribute can be left as-is. I think it may make sense to keep async since this in effect what offloading to Partytown results in. (Or maybe defer actually. I'm not entirely sure of the execution model.) But ultimately, on second thought, let's remove it because if the user didn't add async to begin with, it could be confusing why it is showing up.

@westonruter
Copy link
Member

In 44c469c I addressed a case where a script may already have the defer or async strategy and then its opted-into offloading to web worker. I've added the plugin banner and icon asset.

✅ Good to merge.

@westonruter westonruter merged commit 5c8b6f5 into trunk Aug 6, 2024
16 checks passed
@westonruter westonruter deleted the add/web-worker-offloading branch August 6, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no milestone PRs that do not have a defined milestone for release [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Plugin Proposal A new plugin proposal
4 participants