Update Webpack chunk generation config#1056
Conversation
tofumatt
left a comment
There was a problem hiding this comment.
These changes look good to me from a code/config perspective, and simplifying webpack configs is always a 👍🏻thing.
Gonna try this out on a production site but looks good code-wise!
Also:
Required for WP pre 5.0.
includes/Core/Assets/Assets.php
Outdated
| 'window._googlesitekitBase = ' . wp_json_encode( $inline_data ), | ||
| 'before' | ||
| ); | ||
| wp_localize_script( $handle, '_googlesitekitBase', $this->get_inline_base_data() ); |
There was a problem hiding this comment.
Could we not leave this to use wp_add_inline_script() instead? This one still has a src, and I'd prefer to use the more proper function wherever possible.
tofumatt
left a comment
There was a problem hiding this comment.
Left a comment here about some of the script trickery 😆
| $script . | ||
| '"></scr\' + \'ipt>\' );' | ||
| $script . '?ver=' . GOOGLESITEKIT_VERSION . | ||
| '"></scr\' + \'ipt>\' );' . "\n" |
There was a problem hiding this comment.
What's with the split </script> tag here? If it's just a PHPCS security warning we could silence it instead 😄
There was a problem hiding this comment.
Because this is a document.write call, not splitting up </script> would actually close the enclosing script tag rather than writing it.
There was a problem hiding this comment.
Ah, whoops, right. Thanks 😅
(A comment here explaining that might be handy; it's not immediately obvious why that is the way it is 😄)
Summary
Addresses issue #1054
Relevant technical choices
splitChunkscommonsandvendorchunks are loaded on demandsitekit-vendoris still enqueued as a virtual script/dependencysitekit-commonsis no longer a script which is enqueued, some refactoring was necessarywp_add_inline_scriptfor scripts without ansrcbefore 5.0, so the initialization of data globals has been replaced withwp_localize_scriptwp_localize_scriptis slightly different in that it casts all scalar values of top-level keys to strings. This caused a few E2E tests to fail because a few data request cache keys changed as a result (i.e.falsebecame""forpermaLink). I opted to update the fixtures rather than update the consuming code in several places. In the future, we should nest localized data under a key in the l10n object to prevent this.methods, which caused failures in older versions of WP until they were added (I'm not sure why as this was working fine before). It was as if the endpoint override wasn't registered.Checklist