Complete shared-extension-utils externalization to prevent duplicate store registration#47850
Complete shared-extension-utils externalization to prevent duplicate store registration#47850
Conversation
The `jetpack-modules` and `wordpress-com/plans` stores were being registered multiple times because shared-extension-utils was bundled independently into every consumer's webpack output. This adds the package to the webpack externals map and builds a single UMD bundle via the assets package, following the same pattern used for jetpack-script-data and jetpack-connection. Also adds try/catch guards around store registration calls as defense-in-depth against future regressions.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
The I18nCheckPlugin auto-detects textdomain from the host package's composer.json (jetpack-assets), but the bundled shared-extension-utils code uses its own textdomain. Override I18nCheckPlugin and I18nLoaderPlugin to expect jetpack-shared-extension-utils.
Add phpcs:ignore for TextDomainMismatch and I18nTextDomainFixer since the bundled code uses its own textdomain, not the host package's.
Code Coverage Summary2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
…1437 # Conflicts: # projects/packages/assets/package.json
There was a problem hiding this comment.
Pull request overview
Externalizes @automattic/jetpack-shared-extension-utils to a shared WP script dependency to prevent duplicate bundling that can trigger duplicate Redux store registration errors in the block editor.
Changes:
- Add
@automattic/jetpack-shared-extension-utilsto the webpack dependency-extraction request map and document the behavior. - Build a
jetpack-shared-extension-utilsUMD bundle inpackages/assetsand register it in PHP for use as a WordPress script dependency. - Add store registration guards and update related tests and test setup (including removing now-unneeded console error suppression).
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/tools/check-block-assets.php | Updates the block view-script dependency allowlist. |
| projects/plugins/jetpack/changelog/externalize-shared-extension-utils | Changelog entry for the Jetpack plugin allowlist update. |
| projects/packages/paypal-payments/tests/jest.setup.js | Removes suppression of the “already registered” console error in tests. |
| projects/packages/paypal-payments/changelog/fix-duplicate-store-registration | Changelog entry for PayPal Payments test setup change. |
| projects/packages/assets/webpack.config.js | Adds a new build target to output the shared-extension-utils UMD bundle. |
| projects/packages/assets/src/js/shared-extension-utils.js | Entry module re-exporting @automattic/jetpack-shared-extension-utils for bundling. |
| projects/packages/assets/src/class-shared-extension-utils-assets.php | Registers the new jetpack-shared-extension-utils script handle in PHP. |
| projects/packages/assets/actions.php | Hooks the new asset registration into WordPress load. |
| projects/packages/assets/package.json | Adds the JS dependency and build tooling needed for the new bundle. |
| projects/packages/assets/changelog/externalize-shared-extension-utils | Changelog entry for adding the new UMD bundle/registration. |
| projects/js-packages/webpack-config/src/webpack.js | Externalizes the root shared-extension-utils package and exempts selected subpaths. |
| projects/js-packages/webpack-config/README.md | Documents the new extracted dependency and the subpath exception behavior. |
| projects/js-packages/webpack-config/changelog/externalize-shared-extension-utils | Changelog entry for webpack-config externals update. |
| projects/js-packages/shared-extension-utils/src/store/wordpress-com/index.ts | Adds a guard to prevent duplicate store registration. |
| projects/js-packages/shared-extension-utils/src/store/wordpress-com/test/store-registration.test.js | Adds tests for the wordpress-com store registration guard. |
| projects/js-packages/shared-extension-utils/src/modules-state/index.js | Adds a guard to prevent duplicate modules store registration. |
| projects/js-packages/shared-extension-utils/src/modules-state/test/store-registration.test.js | Adds tests for modules-state store registration guard and initial data population. |
| projects/js-packages/shared-extension-utils/src/hooks/use-module-status/test/index.test.js | Extends mocks to accommodate updated modules-state registration logic. |
| projects/js-packages/shared-extension-utils/changelog/fix-duplicate-store-registration | Changelog entry for the shared-extension-utils guard. |
| projects/js-packages/connection/state/store-holder.jsx | Adds a guard to prevent duplicate connection store registration. |
| projects/js-packages/connection/changelog/investigate-jetpack-1437 | Changelog entry for connection store registration guard. |
| pnpm-lock.yaml | Lockfile updates for new/updated workspace dependencies. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
anomiex
left a comment
There was a problem hiding this comment.
Subpath imports (
/components,/icons,/site-type-utils,/store/wordpress-com) are explicitly excluded from externalization and continue to be bundled. Note:/store/wordpress-comhas store registration side effects at module scope; the others are side-effect free.
I'm skeptical about this part. If we're going to externalize the package, we should probably fully do so instead of allowing subpath imports to bypass it. Especially we shouldn't allow a subpath import of the store that's the reason we're doing this in the first place.
It's possible this means that just the store bit should be its own package, which a non-externalized shared-extension-utils could re-export for back compat. Or (if it's possible) that we should externalize only that one subpath import (and make the stores available only via that subpath import, not via the barrel import).
…1437 # Conflicts: # projects/packages/assets/package.json
|
Reworked in 30741fc based on the feedback in this review round. The earlier version was doing two things at once — partial externalization plus duplicate-registration guards — and the guards were really compensating for the partial part. Backed out the guards and finished the externalization:
On the assets side:
Validation:
Known follow-up: if |
Fixes #34793
Proposed changes
@automattic/jetpack-shared-extension-utilsby dropping theexternal: falseopt-out for/store/wordpress-comand re-exportingwordpressPlansStorefrom the package rootselect()duplicate-registration guards (and their tests); the fix is now packaging behavior rather than local checkspackages/assets, but enqueue its extracted CSS when the shared script handle is already enqueuedjetpack-assetsand add a babel override so bundled strings hit the existing assets-package translation infrastructurejetpackConfigshim for the shared UMD buildconnectionchangelog since that change revertedKnown follow-up: if
Store "jetpack-connection" is already registeredreappears on My Jetpack after backing out the connection-package guard, that's a separate architectural issue (jetpack-connectionis indefaultRequestMapbut not actually built as a UMD) and will be handled in its own PR.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
composer test-phpinprojects/packages/assets.jp test js js-packages/shared-extension-utils.jp build packages/assetsand confirmjetpack-shared-extension-utils.jsand.cssemit.jp build plugins/jetpack --deps..asset.phpincludesjetpack-shared-extension-utilsas a dependency.jetpack-modulesorwordpress-com/plans..has-warning.is-interactiveblock editor styles apply (validates the CSS enqueue hook).