-
Notifications
You must be signed in to change notification settings - Fork 109
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 module #556
Add Web Worker Offloading module #556
Conversation
add_action( 'wp_print_scripts', 'perflab_web_worker_partytown_worker_scripts' ); | ||
|
||
/** | ||
* Helper function to get all scripts tags which has `partytown` dependency. |
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.
Rather than using a partytown dependency, can we look for scripts that have a partytown-worker
strategy (and maybe no dependencies)?
That way partytown could be enabled with:
wp_script_add_data( $handle, 'strategy', 'partytown-worker' );
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.
While it's feasible to execute this, my inclination is towards considering partytown
to be included in the $deps
array as a dependency.
This preference stems from the fact that when a script loads as script[type="text/partytown"]
, it necessitates the presence of partytown
. Thus, referring to it as a direct dependency seems logical.
Additionally, by adhering to the dependency approach, merely registering the partytown
script suffices, as WordPress automatically enqueues it if another script depends on it else it will not be enqueued to the front-end.
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.
That makes sense, however I can also see the use case for the wp_script_add_data
approach. For example, an optimization plugin could use this to specify it wants a specific script loaded with partytown. I'm not sure you could add a dependency dynamically like that (I have to look).
This is fine as is for now, I will check if maybe we can make it work both ways.
@thelovekesh apologies for dropping my attention here: I finally got back to this and left some feedback on the PR; I'd love to help get this over the finish line to land as a module. |
Thanks @adamsilverstein. I will try to resolve the feedback and update you once done. |
293f408
to
94eb6bb
Compare
d1c3417
to
7a92fa7
Compare
ac2c9d0
to
7afe9e9
Compare
@@ -0,0 +1,139 @@ | |||
<?php | |||
/** | |||
* Module Name: Partytown Web Worker |
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.
Do we really want to call the module Partytown?
IMHO usage of the Partytown library is just an implementation detail.
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.
Agreed. A less-specific name would be "Web Worker Offloading". I asked Bard for some alternative names, and it suggested "Threadshift". I like that, but it is trademarked.
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.
That makes sense. If you have a certain name in mind, do let me know, and I will edit 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.
How about Web Worker Offloading for now?
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.
SGTM, some more names:
- Scripts offloading/offloader (in the description add a reference to Web Worker and/or Partytown).
- Third-party JS offloader
- Non-critical JS offloader
I will defer name selection to you.
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.
Those are some good options but I like Web Worker Offloading.
The JS being offloaded may not be third-party. It may also be critical, but just offloaded so it can run in another thread.
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.
* Module Name: Partytown Web Worker | |
* Module Name: Web Worker Offloading |
Since we're currently in the middle of re-plumbing for standalone plugins, I'm going to re-base this branch to a |
@@ -33,6 +34,7 @@ build-cs/composer.lock | |||
node_modules/ | |||
vendor/ | |||
|
|||
modules/js-and-css/partytown-web-worker/assets/js/partytown |
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 will necessitate introducing a .distignore
file to replace the .gitattributes
file as explained in #893 (comment).
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.
Since this would certainly get released as a standalone plugin on Wordpress.org, it seems we'd need to have the plugin actually do something without needing to code. Otherwise, I understand that the plugin review team would reject the plugin as a "framework plugin". I recall there was some discussion of what this could do by default, without there being a clear answer. Would there need to be a UI to select scripts to opt-in? Humm.
@@ -9,6 +9,8 @@ | |||
_x( 'Creates WebP versions for new JPEG image uploads if supported by the server.', 'module description', 'performance-lab' ), | |||
_x( 'Enqueued Assets Health Check', 'module name', 'performance-lab' ), | |||
_x( 'Adds a CSS and JS resource check in Site Health status.', 'module description', 'performance-lab' ), | |||
_x( 'Partytown Web Worker', 'module name', 'performance-lab' ), |
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.
_x( 'Partytown Web Worker', 'module name', 'performance-lab' ), | |
_x( 'Web Worker Offloading', 'module name', 'performance-lab' ), |
@@ -0,0 +1,139 @@ | |||
<?php | |||
/** | |||
* Module Name: Partytown Web Worker |
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.
* Module Name: Partytown Web Worker | |
* Module Name: Web Worker Offloading |
@@ -0,0 +1,139 @@ | |||
<?php | |||
/** | |||
* Test cases for Partytown Web Worker module. |
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.
* Test cases for Partytown Web Worker module. | |
* Test cases for Web Worker Offloading module. |
|
||
$partytown_handles = array(); | ||
foreach ( $wp_scripts->registered as $handle => $script ) { | ||
if ( ! empty( $script->deps ) && in_array( 'partytown', $script->deps, true ) ) { |
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.
Correct me if I'm wrong, but let's say you have an existing script in a theme like:
wp_enqueue_script( 'foo', 'https://example.com/foo.js' );
In order to opt-in this script to use Partytown, you'd have to do this if you wanted to opt-in from a separate plugin:
wp_scripts()->registered['foo']->deps[] = 'partytown';
Is that right?
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.
Yup, that should do the trick.
We could have it automatically apply to analytics scripts where it is expected to work well - although those may not be enqueued with wp_scripts. |
If the user wishes to offload these common scripts, it would be preferable if there was an opt-in form on some sort of options page. Alternatively, the user may utilize this as a framework to offload scripts. There are two scenarios I can think of if the user opts in for script auto-offloading:
|
Just noting this can be updated to use the new standalone plugin structure. |
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.
Approving to continue work in the feature branch
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.
Looking good! Lets keep iterating and testing in the branch.
1f949f4
into
WordPress:feature/web-worker-offloading
Summary
Fixes #176
Please check #271 for previous conversations.
Relevant technical choices
assets/js
folder.partytown_configuration
filter which can be used to update partytown configurations.Screencast
1.mp4
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.