WP Build Polyfills: Inline keyedReducer for @wordpress/notices 5.45+#48743
Conversation
|
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. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
|
Thanks for the second-opinion review — applied all three nits in What changed:
Nothing intentionally skipped. New HEAD: Notes:
|
dhasilva
left a comment
There was a problem hiding this comment.
LGTM. At first I thought "why not bundle wordpress/data?", but that is not possible because it is a registry, so I get now why the PR only extracted the keyed-reducer.
It is a good idea to flag this upstream, this kind of shared functionality should probably live in some other package to avoid this kind of problem.
6182de1 to
8aded42
Compare
@wordpress/notices >= 5.45.0 imports keyedReducer from @wordpress/data, which only added that export in @wordpress/data 10.45.0 (Gutenberg consolidation, WordPress/gutenberg#77364). The polyfill externalizes @wordpress/data to the runtime window.wp.data global so the notices store registers in the shared registry. On sites that still ship an older @wordpress/data (current WordPress Core), the bundled call to window.wp.data.keyedReducer threw at load time, which cascaded into a SnackbarNotices undefined error in the @wordpress/boot polyfill module. Add a webpack loader that rewrites the keyedReducer named import in the @wordpress/notices reducer source to a local copy bundled with the polyfill, leaving every other @wordpress/data symbol externalized as before. A custom loader rather than a Rule.resolve.alias is needed because @wordpress/dependency-extraction-webpack-plugin externalizes the request before module resolution, bypassing per-rule aliases.
…e regression test Address second-opinion review nits on the wp-data keyedReducer inlining fix: - Broaden the loader rule test regex to also match @wordpress/core-data queried-data reducer paths so a future polyfill addition that pulls in core-data does not silently re-hit the same bug. Document the contract (defensive loader + restricted path test) in a comment next to the rule. - Extend the regression test to run against BOTH dev and production (minified) bundle output. The production block runs a fresh webpack build into a sibling tree via WP_BUILD_POLYFILLS_OUTPUT_BASE so it does not clobber build/, then asserts no static wp.data.keyedReducer reference and no stray keyedReducer token survives minification. - Add a test-js composer script (build-then-test) so the CI matrix in .github/workflows/tests.yml actually runs this regression test for PRs touching the package. - Expand the changelog entry: link the upstream consolidation PR (WordPress/gutenberg#77364), frame the loader as a workaround pending a longer-term decision, and note that future polyfill additions importing keyedReducer from @wordpress/data need to extend the rule and test. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The previous commit added a `test-js` script and bumped the package reference SHA in `projects/packages/wp-build-polyfills/composer.json`, which left the five plugin composer.lock files (backup, jetpack, mu-wpcom-plugin, videopress, wpcomsh) out of sync.
772715f to
db10d55
Compare
…48743) * WP Build Polyfills: Inline keyedReducer for @wordpress/notices 5.45+ @wordpress/notices >= 5.45.0 imports keyedReducer from @wordpress/data, which only added that export in @wordpress/data 10.45.0 (Gutenberg consolidation, WordPress/gutenberg#77364). The polyfill externalizes @wordpress/data to the runtime window.wp.data global so the notices store registers in the shared registry. On sites that still ship an older @wordpress/data (current WordPress Core), the bundled call to window.wp.data.keyedReducer threw at load time, which cascaded into a SnackbarNotices undefined error in the @wordpress/boot polyfill module. Add a webpack loader that rewrites the keyedReducer named import in the @wordpress/notices reducer source to a local copy bundled with the polyfill, leaving every other @wordpress/data symbol externalized as before. A custom loader rather than a Rule.resolve.alias is needed because @wordpress/dependency-extraction-webpack-plugin externalizes the request before module resolution, bypassing per-rule aliases. * wp-build-polyfills: broaden keyedReducer loader rule + production-mode regression test Address second-opinion review nits on the wp-data keyedReducer inlining fix: - Broaden the loader rule test regex to also match @wordpress/core-data queried-data reducer paths so a future polyfill addition that pulls in core-data does not silently re-hit the same bug. Document the contract (defensive loader + restricted path test) in a comment next to the rule. - Extend the regression test to run against BOTH dev and production (minified) bundle output. The production block runs a fresh webpack build into a sibling tree via WP_BUILD_POLYFILLS_OUTPUT_BASE so it does not clobber build/, then asserts no static wp.data.keyedReducer reference and no stray keyedReducer token survives minification. - Add a test-js composer script (build-then-test) so the CI matrix in .github/workflows/tests.yml actually runs this regression test for PRs touching the package. - Expand the changelog entry: link the upstream consolidation PR (WordPress/gutenberg#77364), frame the loader as a workaround pending a longer-term decision, and note that future polyfill additions importing keyedReducer from @wordpress/data need to extend the rule and test. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * wp-build-polyfills: regenerate consumer composer.lock files The previous commit added a `test-js` script and bumped the package reference SHA in `projects/packages/wp-build-polyfills/composer.json`, which left the five plugin composer.lock files (backup, jetpack, mu-wpcom-plugin, videopress, wpcomsh) out of sync. * Add changelog entries for regenerated composer.lock files --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Fixes runtime breakage observed in the #48405 smoke test (#48405 (comment)).
Proposed changes
wp-data-keyed-reducer-loader.js) that rewritesimport { keyedReducer } from '@wordpress/data'inside@wordpress/notices' bundledstore/reducer.{mjs,cjs}to import from a local copy of the helper.src/internal/keyed-reducer.js(byte-identical to upstream's@wordpress/data10.45.0keyedReducer).wp.data.keyedReducerand (b) the bundledcore/noticesreducer behaves correctly with a stubbedwindow.wp.datathat omitskeyedReducer.The fix is a no-op against today's pinned
@wordpress/notices5.44.0 (its bundled reducer uses an internalon-sub-keyhelper instead) — the loader'stestonly matches the file the upstream refactor moves the import into. Other@wordpress/datasymbols (createReduxStore,register,useDispatch,useSelect) continue to be externalized through the standard@wordpress/dependency-extraction-webpack-pluginpath so the notices store keeps registering in the sharedwindow.wp.dataregistry.Root cause
@wordpress/data10.45.0 addedkeyedReduceras a public export and Gutenberg consolidated the duplicated copies previously maintained in@wordpress/core-dataand@wordpress/notices(WordPress/gutenberg#77364).@wordpress/notices5.45.0'sstore/reducernow does:wp-build-polyfills' IIFE bundle for@wordpress/noticesexternalizes@wordpress/datato the runtimewindow.wp.dataglobal so the notices store registers in the shared registry. WordPress Core today still ships@wordpress/databelow 10.45.0, so the bundledwp.data.keyedReducer(...)call at module load throws "n(...).keyedReducer is not a function" — and once the IIFE has thrown,window.wp.noticesis left unassigned, so the boot module's downstream<SnackbarNotices />render trips on "can't access property "SnackbarNotices", r is undefined".A
Rule.resolve.aliasmapping for@wordpress/datadoesn't help: the DEP plugin'sexternalscallback short-circuits the request before module resolution, so per-rule alias entries for@wordpress/dataare bypassed. Rewriting the import in source (this PR's loader) is the only path that survives the externals pipeline.Why not pin
@wordpress/noticesto 5.44Renovate would re-bump it next week and the same failure mode would recur. Inlining
keyedReduceris robust against future minor-version bumps of@wordpress/noticesfor the same regression class.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
@wordpress/noticesresolves to 5.45.0): on the branch, fetchpnpm-lock.yamlfrom Update wordpress monorepo #48405's HEAD (commit67217d91) into the worktree root, thenpnpm install --prefer-offline.pnpm jetpack build packages/wp-build-polyfills.cd projects/packages/wp-build-polyfills && pnpm test— all 12 tests should pass.grep keyedReducer build/scripts/notices/index.js. The reducer call should reference an inlined local export, not thewp.dataexternal.To reproduce the failure without the fix, comment out the
wpDataKeyedReducerRuleline fromwebpack.config.js, rebuild, and re-run the tests — the regression test "must produce a notices reducer that works withoutwp.data.keyedReducer" fails with aTypeErrorfrom inside the vm-evaluated bundle.After this lands, #48405 should rebase / re-run its smoke test on
advanced-krill.jurassic.ninja(or successor) and the Forms admin page should load cleanly.