-
Notifications
You must be signed in to change notification settings - Fork 106
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
Integrate with WooCommerce to offload Google Analytics to Web Worker #1563
Changes from all commits
eb523a1
f430916
84fc204
796996e
ac4744f
c2d73f0
844884c
fc70dec
5986738
d188626
805ec70
efe3506
e9ac924
5a7d421
ac9c55f
a0af3d0
649e617
74b93c7
89ea382
2f38b13
41434a2
cfea71a
db9d10e
998d19f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
"optimization-detective", | ||
"performance-lab", | ||
"speculation-rules", | ||
"web-worker-offloading", | ||
"webp-uploads" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,70 @@ | ||||||
<?php | ||||||
/** | ||||||
* Helpers for Web Worker Offloading. | ||||||
* | ||||||
* @since 0.1.0 | ||||||
* @package web-worker-offloading | ||||||
*/ | ||||||
|
||||||
if ( ! defined( 'ABSPATH' ) ) { | ||||||
exit; // Exit if accessed directly. | ||||||
} | ||||||
|
||||||
/** | ||||||
* Gets configuration for Web Worker Offloading. | ||||||
* | ||||||
* @since 0.1.0 | ||||||
* @link https://partytown.builder.io/configuration | ||||||
* @link https://github.com/BuilderIO/partytown/blob/b292a14047a0c12ca05ba97df1833935d42fdb66/src/lib/types.ts#L393-L548 | ||||||
* | ||||||
* @return array<string, mixed> Configuration for Partytown. | ||||||
*/ | ||||||
function wwo_get_configuration(): array { | ||||||
$config = array( | ||||||
'lib' => wp_parse_url( plugin_dir_url( __FILE__ ), PHP_URL_PATH ) . 'build/', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There used to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, because it was empty so it wasn't having any purpose. |
||||||
); | ||||||
|
||||||
if ( WP_DEBUG && SCRIPT_DEBUG ) { | ||||||
$config['debug'] = true; | ||||||
} | ||||||
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about always having this key set, and making the conditional only around whether it's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, because it defaults to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's probably negligible though for performance. I'd say initializing properties here makes it easier to understand what's available, also since this PHP layer is entirely separate from the actual JS API where this is consumed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure. If someone wants to know what is available they should look at the docs which are linked in the source code: performance/plugins/web-worker-offloading/helper.php Lines 17 to 18 in db9d10e
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to have to supply the default values for very single configuration option (many of which are under-documented in Partytown). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. Let's leave this as is then. |
||||||
|
||||||
/** | ||||||
* Add configuration for Web Worker Offloading. | ||||||
* | ||||||
* Many of the configuration options are not documented publicly, so refer to the TypeScript definitions. | ||||||
* Additionally, not all of the configuration options (e.g. functions) can be serialized as JSON and must instead be | ||||||
* defined in JavaScript instead. To do so, use the following PHP code instead of filtering `wwo_configuration`: | ||||||
* | ||||||
* add_action( | ||||||
* 'wp_enqueue_scripts', | ||||||
* function () { | ||||||
* wp_add_inline_script( | ||||||
* 'web-worker-offloading', | ||||||
* <<<JS | ||||||
* window.partytown = { | ||||||
* ...(window.partytown || {}), | ||||||
* resolveUrl: (url, location, type) => { | ||||||
* if (type === 'script') { | ||||||
* const proxyUrl = new URL('https://my-reverse-proxy.example.com/'); | ||||||
* proxyUrl.searchParams.append('url', url.href); | ||||||
* return proxyUrl; | ||||||
* } | ||||||
* return url; | ||||||
* }, | ||||||
* }; | ||||||
* JS, | ||||||
* 'before' | ||||||
* ); | ||||||
* } | ||||||
* ); | ||||||
* | ||||||
* There are also many configuration options which are not documented, so refer to the TypeScript definitions. | ||||||
* | ||||||
* @since 0.1.0 | ||||||
* @link https://partytown.builder.io/configuration | ||||||
* @link https://github.com/BuilderIO/partytown/blob/b292a14047a0c12ca05ba97df1833935d42fdb66/src/lib/types.ts#L393-L548 | ||||||
* | ||||||
* @param array<string, mixed> $config Configuration for Partytown. | ||||||
*/ | ||||||
return (array) apply_filters( 'wwo_configuration', $config ); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to add some sanitization here. For example, since several supported properties/keys are arrays and several plugins/sources may add similar entries to them, we could sanitize these arrays using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would agree with you, but I think sanitizing for this initial version could go down a rabbit hole. The number of possible configuration options is large and the value types can be complex: https://github.com/BuilderIO/partytown/blob/main/src/lib/types.ts We could implement a JSON Schema to mirror the TypeScript types and use this to provide usage warnings. However, I think this validation would be better done inside Partytown itself where the types are being defined. In the case of } else if (webWorkerCtx.$config$.mainWindowAccessors?.includes(propName)) {
return getter(this, [propName]); So I think we should consider what validation and sanitization we do as part of this plugin for a subsequent release. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it depends also on how we want to use their configuration options. We don't need to holistically capture everything Partytown supports, that would indeed be a rabbit hole. But for instance, it seems that As long as all of these 4 arguments only support an array of strings, and as long as duplicate values are not a problem, it's probably okay not to sanitize anything. So we can move ahead with this as is, but I think we should look out for when things are more complicated to use, or could be of more than one type, and especially then we should do some sanitization for how we want those arguments to be used. |
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,35 +10,11 @@ | |
exit; // Exit if accessed directly. | ||
} | ||
|
||
/** | ||
* Gets configuration for Web Worker Offloading. | ||
* | ||
* @since 0.1.0 | ||
* @link https://partytown.builder.io/configuration | ||
* | ||
* @return array{ debug?: bool, forward?: non-empty-string[], lib: non-empty-string, loadScriptsOnMainThread?: non-empty-string[], nonce?: non-empty-string } Configuration for Partytown. | ||
*/ | ||
function wwo_get_configuration(): array { | ||
$config = array( | ||
'lib' => wp_parse_url( plugin_dir_url( __FILE__ ), PHP_URL_PATH ) . 'build/', | ||
'forward' => array(), | ||
); | ||
|
||
/** | ||
* Add configuration for Web Worker Offloading. | ||
* | ||
* @since 0.1.0 | ||
* @link https://partytown.builder.io/configuration | ||
* | ||
* @param array{ debug?: bool, forward?: non-empty-string[], lib: non-empty-string, loadScriptsOnMainThread?: non-empty-string[], nonce?: non-empty-string } $config Configuration for Partytown. | ||
*/ | ||
return apply_filters( 'wwo_configuration', $config ); | ||
} | ||
|
||
/** | ||
* Registers defaults scripts for Web Worker Offloading. | ||
* | ||
* @since 0.1.0 | ||
* @access private | ||
* | ||
* @param WP_Scripts $scripts WP_Scripts instance. | ||
*/ | ||
|
@@ -59,7 +35,7 @@ function wwo_register_default_scripts( WP_Scripts $scripts ): void { | |
$scripts->add_inline_script( | ||
'web-worker-offloading', | ||
sprintf( | ||
'window.partytown = %s;', | ||
'window.partytown = {...(window.partytown || {}), ...%s};', | ||
wp_json_encode( wwo_get_configuration() ) | ||
), | ||
'before' | ||
|
@@ -72,12 +48,8 @@ function wwo_register_default_scripts( WP_Scripts $scripts ): void { | |
/** | ||
* Prepends web-worker-offloading to the list of scripts to print if one of the queued scripts is offloaded to a worker. | ||
* | ||
* This also marks their strategy as `async`. This is needed because scripts offloaded to a worker thread can be | ||
* considered async. However, they may include `before` and `after` inline scripts that need sequential execution. Once | ||
* marked as async, `filter_eligible_strategies()` determines if the script is eligible for async execution. If so, it | ||
* will be offloaded to the worker thread. | ||
* | ||
* @since 0.1.0 | ||
* @access private | ||
* | ||
* @param string[]|mixed $script_handles An array of enqueued script dependency handles. | ||
* @return string[] Script handles. | ||
|
@@ -88,61 +60,62 @@ function wwo_filter_print_scripts_array( $script_handles ): array { | |
if ( true === (bool) $scripts->get_data( $handle, 'worker' ) ) { | ||
$scripts->set_group( 'web-worker-offloading', false, 0 ); // Try to print in the head. | ||
array_unshift( $script_handles, 'web-worker-offloading' ); | ||
|
||
// TODO: This should be reconsidered because scripts needing to be offloaded will often have after scripts. See <https://github.com/WordPress/performance/pull/1497/files#r1733538721>. | ||
if ( false === wp_scripts()->get_data( $handle, 'strategy' ) ) { | ||
wp_script_add_data( $handle, 'strategy', 'async' ); // The 'defer' strategy would work as well. | ||
wp_script_add_data( $handle, 'wwo_strategy_added', true ); | ||
} | ||
Comment on lines
-92
to
-96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inline after scripts are often used with analytics, and they need to be offloaded to a worker (maybe always?). |
||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We |
||
} | ||
} | ||
return $script_handles; | ||
} | ||
add_filter( 'print_scripts_array', 'wwo_filter_print_scripts_array' ); | ||
add_filter( 'print_scripts_array', 'wwo_filter_print_scripts_array', PHP_INT_MAX ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The priority is increased to |
||
|
||
/** | ||
* Updates script type for handles having `web-worker-offloading` as dependency. | ||
* | ||
* @since 0.1.0 | ||
* @access private | ||
* | ||
* @param string|mixed $tag Script tag. | ||
* @param string $handle Script handle. | ||
* @return string|mixed Script tag with type="text/partytown" for eligible scripts. | ||
*/ | ||
function wwo_update_script_type( $tag, string $handle ) { | ||
if ( | ||
is_string( $tag ) && | ||
is_string( $tag ) | ||
&& | ||
(bool) wp_scripts()->get_data( $handle, 'worker' ) | ||
) { | ||
$html_processor = new WP_HTML_Tag_Processor( $tag ); | ||
|
||
while ( $html_processor->next_tag( array( 'tag_name' => 'SCRIPT' ) ) ) { | ||
if ( $html_processor->get_attribute( 'id' ) !== "{$handle}-js" ) { | ||
continue; | ||
} | ||
if ( null === $html_processor->get_attribute( 'async' ) && null === $html_processor->get_attribute( 'defer' ) ) { | ||
_doing_it_wrong( | ||
'wwo_update_script_type', | ||
esc_html( | ||
sprintf( | ||
/* translators: %s: script handle */ | ||
__( 'Unable to offload "%s" script to a worker. Script will continue to load in the main thread.', 'web-worker-offloading' ), | ||
$handle | ||
) | ||
), | ||
'Web Worker Offloading 0.1.0' | ||
); | ||
} else { | ||
if ( $html_processor->get_attribute( 'id' ) === "{$handle}-js" ) { | ||
$html_processor->set_attribute( 'type', 'text/partytown' ); | ||
$tag = $html_processor->get_updated_html(); | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only one tag should have a given ID, so once we found it, we can bail. |
||
} | ||
if ( true === wp_scripts()->get_data( $handle, 'wwo_strategy_added' ) ) { | ||
$html_processor->remove_attribute( 'async' ); | ||
$html_processor->remove_attribute( 'data-wp-strategy' ); | ||
} | ||
$tag = $html_processor->get_updated_html(); | ||
} | ||
} | ||
|
||
return $tag; | ||
} | ||
add_filter( 'script_loader_tag', 'wwo_update_script_type', 10, 2 ); | ||
|
||
/** | ||
* Filters inline script attributes to offload to a worker if the script has been opted-in. | ||
* | ||
* @since 0.1.0 | ||
* @access private | ||
* | ||
* @param array<string, mixed>|mixed $attributes Attributes. | ||
* @return array<string, mixed> Attributes. | ||
*/ | ||
function wwo_filter_inline_script_attributes( $attributes ): array { | ||
$attributes = (array) $attributes; | ||
if ( | ||
isset( $attributes['id'] ) | ||
&& | ||
1 === preg_match( '/^(?P<handle>.+)-js-(?:before|after)$/', $attributes['id'], $matches ) | ||
&& | ||
(bool) wp_scripts()->get_data( $matches['handle'], 'worker' ) | ||
) { | ||
$attributes['type'] = 'text/partytown'; | ||
} | ||
return $attributes; | ||
} | ||
add_filter( 'wp_inline_script_attributes', 'wwo_filter_inline_script_attributes' ); |
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 just moved this function from
hooks.php
tohelpers.php