-
Notifications
You must be signed in to change notification settings - Fork 101
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
Conversation
@felixarntz @adamsilverstein This isn't ready for a full review, but I wanted to get a pre-review from you guys as the overall foundation is in place here. Current diff with WooCommerce after formatting with Prettier--- /tmp/before.html 2024-09-27 15:07:35.737764370 -0700
+++ /tmp/after.html 2024-09-27 15:09:10.461767260 -0700
@@ -2196,7 +2196,10 @@
visibility: visible;
}
</style>
- <script id="woocommerce-google-analytics-integration-gtag-js-after">
+ <script
+ id="woocommerce-google-analytics-integration-gtag-js-after"
+ type="text/partytown"
+ >
/* Google Analytics for WooCommerce (gtag.js) */
window.dataLayer = window.dataLayer || [];
function gtag() {
@@ -3185,6 +3188,144 @@
document.body.className = c;
})();
</script>
+ <script id="web-worker-offloading-js-before">
+ window.partytown = {
+ lib: "\/wp-content\/plugins\/web-worker-offloading\/build\/",
+ forward: ["dataLayer.push"],
+ };
+ </script>
+ <script id="web-worker-offloading-js-after">
+ const t = { preserveBehavior: !1 },
+ e = (e) => {
+ if ("string" == typeof e) return [e, t];
+ const [n, r = t] = e;
+ return [n, { ...t, ...r }];
+ },
+ n = Object.freeze(
+ ((t) => {
+ const e = new Set();
+ let n = [];
+ do {
+ Object.getOwnPropertyNames(n).forEach((t) => {
+ "function" == typeof n[t] && e.add(t);
+ });
+ } while ((n = Object.getPrototypeOf(n)) !== Object.prototype);
+ return Array.from(e);
+ })(),
+ );
+ !(function (t, r, o, i, a, s, c, d, l, p, u = t, f) {
+ function h() {
+ f ||
+ ((f = 1),
+ "/" ==
+ (c = (s.lib || "/~partytown/") + (s.debug ? "debug/" : ""))[0] &&
+ ((l = r.querySelectorAll('script[type="text/partytown"]')),
+ i != t
+ ? i.dispatchEvent(new CustomEvent("pt1", { detail: t }))
+ : ((d = setTimeout(v, 1e4)),
+ r.addEventListener("pt0", w),
+ a
+ ? y(1)
+ : o.serviceWorker
+ ? o.serviceWorker
+ .register(c + (s.swPath || "partytown-sw.js"), {
+ scope: c,
+ })
+ .then(function (t) {
+ t.active
+ ? y()
+ : t.installing &&
+ t.installing.addEventListener(
+ "statechange",
+ function (t) {
+ "activated" == t.target.state && y();
+ },
+ );
+ }, console.error)
+ : v())));
+ }
+ function y(e) {
+ (p = r.createElement(e ? "script" : "iframe")),
+ (t._pttab = Date.now()),
+ e ||
+ ((p.style.display = "block"),
+ (p.style.width = "0"),
+ (p.style.height = "0"),
+ (p.style.border = "0"),
+ (p.style.visibility = "hidden"),
+ p.setAttribute("aria-hidden", !0)),
+ (p.src =
+ c +
+ "partytown-" +
+ (e ? "atomics.js?v=0.10.2" : "sandbox-sw.html?" + t._pttab)),
+ r.querySelector(s.sandboxParent || "body").appendChild(p);
+ }
+ function v(n, o) {
+ for (
+ w(),
+ i == t &&
+ (s.forward || []).map(function (n) {
+ const [r] = e(n);
+ delete t[r.split(".")[0]];
+ }),
+ n = 0;
+ n < l.length;
+ n++
+ )
+ ((o = r.createElement("script")).innerHTML = l[n].innerHTML),
+ (o.nonce = s.nonce),
+ r.head.appendChild(o);
+ p && p.parentNode.removeChild(p);
+ }
+ function w() {
+ clearTimeout(d);
+ }
+ (s = t.partytown || {}),
+ i == t &&
+ (s.forward || []).map(function (r) {
+ const [o, { preserveBehavior: i }] = e(r);
+ (u = t),
+ o.split(".").map(function (e, r, o) {
+ var a;
+ u = u[o[r]] =
+ r + 1 < o.length
+ ? u[o[r]] || ((a = o[r + 1]), n.includes(a) ? [] : {})
+ : (() => {
+ let e = null;
+ if (i) {
+ const { methodOrProperty: n, thisObject: r } = ((
+ t,
+ e,
+ ) => {
+ let n = t;
+ for (let t = 0; t < e.length - 1; t += 1)
+ n = n[e[t]];
+ return {
+ thisObject: n,
+ methodOrProperty:
+ e.length > 0 ? n[e[e.length - 1]] : void 0,
+ };
+ })(t, o);
+ "function" == typeof n &&
+ (e = (...t) => n.apply(r, ...t));
+ }
+ return function () {
+ let n;
+ return (
+ e && (n = e(arguments)),
+ (t._ptf = t._ptf || []).push(o, arguments),
+ n
+ );
+ };
+ })();
+ });
+ }),
+ "complete" == r.readyState
+ ? h()
+ : (t.addEventListener("DOMContentLoaded", h),
+ t.addEventListener("load", h));
+ })(window, document, navigator, top, window.crossOriginIsolated);
+ </script>
<script
src="http://localhost:8888/wp-includes/js/dist/vendor/wp-polyfill.js?ver=3.15.0"
id="wp-polyfill-js"
@@ -3260,7 +3401,7 @@
id="wc-mini-cart-block-frontend-js"
></script>
<script
- async
+ type="text/partytown"
src="https://www.googletagmanager.com/gtag/js?id=G-12345"
id="google-tag-manager-js"
data-wp-strategy="async" I'm not entirely sure yet why |
Ah, the reason is that WooCommerce is doing this: /**
* Add async to script tags with defined handles.
*
* @param string $tag HTML for the script tag.
* @param string $handle Handle of script.
* @param string $src Src of script.
* @return string
*/
public function async_script_loader_tags( $tag, $handle, $src ) {
if ( ! in_array( $handle, array( 'google-tag-manager' ), true ) ) {
return $tag;
}
// If script was output manually in wp_head, abort.
if ( did_action( 'woocommerce_gtag_snippet' ) ) {
return '';
}
return str_replace( '<script src', '<script async src', $tag );
} This is forcing the injection of the The reason why WooCommerce's In the end, this doesn't matter at all because a Partytown-run script is effectively also async. |
…rint_scripts_array filtering
* | ||
* @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 { |
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
to helpers.php
// 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 ); | ||
} |
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.
Inline after scripts are often used with analytics, and they need to be offloaded to a worker (maybe always?).
wp_script_add_data( $handle, 'strategy', 'async' ); // The 'defer' strategy would work as well. | ||
wp_script_add_data( $handle, 'wwo_strategy_added', true ); | ||
} | ||
break; |
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.
We break
because we only need to prepend the WWO script when encountering the first script that is to be offloaded to a worker.
} | ||
} | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The priority is increased to PHP_INT_MAX
so that functions like wwo_mark_scripts_for_offloading()
can add print_scripts_array
filters to mark scripts for worker offloading easily at earlier priorities.
$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 comment
The 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.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
If this is approved, I'm thinking to submit it to the plugin directory for a Monday release. |
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.
@westonruter This mostly looks good, just a few questions / suggestions.
*/ | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
There used to be forward
in here, can you clarify why you removed 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.
Yes, because it was empty so it wasn't having any purpose.
$configuration['mainWindowAccessors'][] = 'wp'; // Because woocommerce-google-analytics-integration needs to access wp.i18n. | ||
$configuration['mainWindowAccessors'][] = 'ga4w'; // Because woocommerce-google-analytics-integration needs to access window.ga4w. | ||
$configuration['globalFns'][] = 'gtag'; // Because gtag() is defined in one script and called in another. | ||
$configuration['forward'][] = 'dataLayer.push'; // Because the Partytown integration has this in its example config. |
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.
Don't these cause PHP notices because the keys they add to are not initialized as arrays?
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.
No it doesn't: https://3v4l.org/YPITA
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.
In fact, this also doesn't cause any notice:
$foo['bar']['baz'][] = 'quux';
It results in $foo
being defined as:
array(1) {
["bar"]=>
array(1) {
["baz"]=>
array(1) {
[0]=>
string(4) "quux"
}
}
}
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.
Hmm, I learned something today. :)
* | ||
* @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 comment
The 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 array_unique()
. Just thinking about what this PR implements for WooCommerce for example allowlisting wp
, I'm sure lots of scripts would need that.
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 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 mainWindowAccessors
, there's no need for array_unique()
because Partytown is using the value like this:
} 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 comment
The 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 mainWindowAccessors
will be relevant in how we would use Partytown for quite a few integrations, so we could do some sanitization there where it makes sense. In other words, I think we should do this based on demand.
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.
if ( WP_DEBUG && SCRIPT_DEBUG ) { | ||
$config['debug'] = 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.
How about always having this key set, and making the conditional only around whether it's true
or false
?
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.
Well, because it defaults to false
anyway, and by not setting it we avoid sending (a tiny amount) of bytes down the wire.
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'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 comment
The 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
* @link https://partytown.builder.io/configuration | |
* @link https://github.com/BuilderIO/partytown/blob/b292a14047a0c12ca05ba97df1833935d42fdb66/src/lib/types.ts#L393-L548 |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. Let's leave this as is then.
…it is not yet ready for release." This reverts commit 16222c1.
Summary
This integrates WooCommerce's Google Analytics integration with Partytown to offload
gtag()
to a worker. Inline scripts now no longer block a registered script from being offloaded to a worker; any associated inline scripts are also offloaded to a worker. It also improves the way that the configuration is managed and fleshes out the README for the initial release.See #1455 (integration with Site Kit and Rank Math can be done in separate PRs)
Diff of Prettier-formatted page with plugin active
Looking at network logs after click an Add to Cart button...
Before:
After:
As can be seen here, with the WWO plugin active, the HTTP request to Google Analytics is happening in a worker.
How to test
/wp-admin/admin.php?page=wc-settings&tab=integration§ion=google_analytics
and supply some GA ID, for example:Ideally you would also look at Google Analytics itself to see that the events are being tracked as expected, but this is beyond my off-hand knowledge.
Relevant technical choices
WooCommerce needs to access
wp.i18n
from a worker-offloaded script, but unfortunately Partytown skips serializing any object properties that begin with_
, meaning callingwp.i18n.__()
will throw an error. I've raised this issue as QwikDev/partytown#629 and implemented a workaround in my fork westonruter/partytown#1. I've updated the@builder.io/partytown
npm dependency to use my fork.See PR comments for additional annotations.