-
Notifications
You must be signed in to change notification settings - Fork 107
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 partytown support to run scripts in Worker Thread #271
Conversation
@thelovekesh Can you please add [Focus] and [Type] labels, as well as a |
@bethanylang I have updated the PR description but I don't have access to add the label in PR. Can you please do that for me? |
); | ||
} | ||
} | ||
add_action( 'wp_print_scripts', 'web_worker_partytown_worker_scripts' ); |
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.
Instead of wp_print_scripts
, this should perhaps do wp_head
. Otherwise, It can run multiple times as found in ampproject/amp-wp#6643.
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.
Or maybe this should be refactored to have a script_loader_tag
filter added at the top level.
modules/javascript/web-worker/assets/js/partytown/debug/partytown.js
Outdated
Show resolved
Hide resolved
@westonruter What if we consider this feature to be integrated into the PWA plugin instead of this one? By both means, this feature can be suggested to be merged in Core but from the PWA plugin, it will be more tested with PWA use cases as well. What do you think? |
I think it makes sense as part of the Performance plugin, especially since the scope doesn't conflict. |
The goal of the performance plugin is to serve as a feature plugin for things that will - eventually (hopefully) - land in WP Core. If the chances of something like this landing in core are slim because of WP's established attitude towards implementations like this, then perhaps there are alternatives we can explore? |
I don't think that is something we need to decide now. How Partytown will mature is hard to predict. Right now, it's just promising, but it is already recommended in Google's optimizing third-party scripts guide. Having it here as a module means it will be exposed to more people than with a standalone plugin, and it will help us understand the best way to integrate it with WordPress. And one could even argue that the other way around is also true: people looking for Partytown in WordPress will find and install this plugin, and the rest of the modules will get more exposure and testing. Over time, we can decide if the best approach is to use an external plugin or a pre-registered dependency in core. |
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've been testing this and everything seems to work great. Great work @thelovekesh 🙂
Maybe in the future, we can add a debug
toggle in the settings, but it can easily be activated using the configuration filter, so not a problem.
function partytown_debug_mode ( $config ) {
$config["debug"] = true;
return $config;
}
add_filter( 'partytown_configuration', 'partytown_debug_mode' );
@thelovekesh, any progress here? Are you planning on finishing this PR? 🙂 |
@luisherranz yes and it's on my todo. Based on the above discussion I don't have much work to do on this PR. Once it got some reviews and ideas I will make changes accordingly. |
Awesome. Thanks @thelovekesh! |
function( $tag, $handle, $src ) use ( $partytown_handle ) { | ||
if ( $handle === $partytown_handle ) { | ||
$create_script_tag = sprintf( | ||
'<script type="text/partytown" src="%1s" id="%2s"></script>', |
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.
why create a script tag from scratch? this could loose other attributes added by other filters. Instead can we replace (or set) just the type
? Is changing the ID required/expected?
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.
Overall this looks really good! I left some code feedback for things that could be improved.
Some questions:
- are there any scripts loaded by core or core themes that are candidates for
partytown
loading? - how would this get used, do we expect plugin authors to add an integration when the Performance Lab plugin is active, or do we want to try to build some integrations for top plugins where we know this could be helpful?
- how does this affect performance? it would be great to run some lab tests with some common candidates like analytics to measure the impact of this change.
efb9b70
to
55213ba
Compare
Closed accidently due to commits dropped from the patch branch. I have opened a new one here - #556 |
Summary
Fixes #176
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.