Search Blocks: enable @wordpress/i18n in the IAPI view bundle (SEARCH-168)#48551
Search Blocks: enable @wordpress/i18n in the IAPI view bundle (SEARCH-168)#48551
Conversation
This comment has been minimized.
This comment has been minimized.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3cf1b71 to
74f8b90
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
74f8b90 to
ef8151e
Compare
Review-cycle summary —
|
| Source | Comment | Resolution |
|---|---|---|
| claude[bot] | (#IC_kwDOAOho7M8AAAABBVwvcA) inverted "first-wins" docblock; missing collect_locale_data() test coverage; explicit DEP fall-through; Plural-Forms casing; _nx fallback signature |
Docblock rewritten to match actual semantics (ef8151e); build_locale_data_payload() extracted with two new PHPUnit cases; requestToExternalModule now leads with an explicit early-return; casing handled via ?? fallback to the Western 2-form rule; shim fallback signature scoped to the unreachable wp-i18n-missing case. |
| copilot-swe-agent | (#IC_kwDOAOho7M8AAAABBVyIQQ) same three actionable items + changelog Type: changed → Type: fixed |
Same commit ef8151e; changelog Type updated. |
| claude[bot] (re-review) | (#IC_kwDOAOho7M8AAAABBV05Wg) "Ready to merge." | No action — confirmation. |
| copilot-swe-agent (re-review) | (#IC_kwDOAOho7M8AAAABBV1YTA) "Looks good to merge." | No action — confirmation. |
| jp-launch-control | (#IC_kwDOAOho7M8AAAABBVz1Cw) Code-coverage delta (-6.80% in class-search-blocks.php; new i18n-shim.js at 0/16) |
Coverage check itself is pass. The drop is enqueue_i18n_runtime() / register_i18n_module() / collect_locale_data() wrapping WP registry functions (not unit-testable without a WP bootstrap), and i18n-shim.js is a browser-only ESM shim whose runtime path is exercised through result-utils.test.js / store.test.js against the real @wordpress/i18n npm package. Both reviewers explicitly accepted the gap as "no action needed." |
Unaddressed (flagged for owner): None.
CI: all required checks passing (Test plugin upgrades is pending, deploy-preview only — not blocking).
…-168) Externalize @wordpress/i18n to a script-module reference (via DEP's requestToExternalModule) and register a tiny ESM shim that re-exports window.wp.i18n as the @wordpress/i18n module. The classic wp-i18n script is enqueued on every search-blocks page so window.wp.i18n is populated synchronously before any deferred module evaluates; translations are inlined as setLocaleData() against PHP's already-loaded gettext entries for jetpack-search-pkg, so __() / _n() / sprintf() return localized strings on first paint without depending on per-handle .json files. Use the new path in: - result-utils.js buildRatingAriaLabel — fixes the SEARCH-168 frontend product-rating aria label (was hardcoded English when used outside the editor preview). - store/index.js computeResultsCountText — replaces the PHP-seeded state.strings reads with native __()/_n()/sprintf(). - active-filters/view.js activePills — same swap. Drop the now-unused build_initial_strings() PHP seed and the test assertions that pinned its shape.
…hangelog Type - Fix the inverted "first-wins" claim in register_i18n_module()'s docblock: ours wins because we register first; flag the maintenance hazard if WP core later ships a native @wordpress/i18n module (raised by both Copilot and claude[bot]). - Extract build_locale_data_payload() out of collect_locale_data() and cover it with two unit tests (Jed shape from a fake Translations, default Plural-Forms fallback when the header is missing). Closes the gap both reviewers flagged for the new PHP code. - Make webpack.blocks.config.js's requestToExternalModule explicit: early-return for non-i18n requests so the DEP fall-through is visible at a glance (claude[bot] nit). - Update changelog Type from "changed" to "fixed" to match the SEARCH-168 framing (Copilot nit).
… setLocaleData The first cut of SEARCH-168 inlined a custom `setLocaleData()` payload from PHP — duplicating what `Automattic\Jetpack\Assets`' `wp-jp-i18n-loader` script + `@wordpress/jp-i18n-loader`'s `downloadI18n()` already do for classic-script bundles. Switch to the established pipeline: - `enqueue_i18n_runtime()` now just enqueues `wp-i18n` and `wp-jp-i18n-loader` (registered by `Automattic\Jetpack\Assets::wp_default_scripts_hook` with the locale + domainPath state already populated). Drop the bespoke `collect_locale_data()` / `build_locale_data_payload()` PHP helpers and their PHPUnit cases. - `store/i18n-bootstrap.js` calls `wp.jpI18nLoader.downloadI18n( bundlePath, 'jetpack-search-pkg', 'plugin' )` from each entry that has its own translatable strings (`store/index.js` and `active-filters/view.js`). jp-i18n-loader hashes the bundle path against `state.domainPaths` to match Jetpack's per-handle .json filenames, then `setLocaleData()`s the result into the live `wp.i18n` runtime — same path `@automattic/i18n-loader-webpack-plugin` injects into classic-script builds. - The shim, `register_i18n_module()`, and the `'module @wordpress/i18n'` external stay: webpack still rewrites `import '@wordpress/i18n'` to a static script-module reference under the canonical `@wordpress/i18n` ID, and the shim re-exports `window.wp.i18n` by reference so calls go through the same instance jp-i18n-loader writes translations into. `var wp.i18n` externalization was rejected during this refactor — DEP records the externalized value (not the original request) in module mode, which leaks `wp.i18n` into the .asset.php `dependencies` array; WP's script-module registry then silently refuses to enqueue any view bundle whose declared deps reference an unresolvable module ID, breaking every block that uses `__()`.
ef8151e to
248bef1
Compare
Fixes SEARCH-168
Why
A shopper using a screen reader on a non-English Jetpack Search store currently hears the product rating in English ("4.5 out of 5 stars based on 42 reviews") even though everything else on the page is translated. The fix gives the Interactivity API view bundle a real
@wordpress/i18nit can call, so the rating aria-label — and every other JS-emitted view-bundle string — picks up the page locale.This also unblocks any future Jetpack Search block code that wants to call
__()/_n()/sprintf()directly instead of detouring throughstate.strings.*seeded from PHP.Proposed changes
@wordpress/i18nas a script-module reference.tools/webpack.blocks.config.jspasses arequestToExternalModulecallback that returns'module @wordpress/i18n'so the IAPI bundle emits a hoisted staticimport * from "@wordpress/i18n"(same pattern DEP already uses for@wordpress/interactivity). Without this, DEP throws "Attempted to use WordPress script in a module" — core only registers@wordpress/interactivity(and a11y / router) as script modules.src/search-blocks/store/i18n-shim.js. Re-exports the live functions onwindow.wp.i18n(__,_x,_n,_nx,sprintf,isRTL,hasTranslation,setLocaleData) with identity fallbacks ifwp-i18nsomehow didn't load. Built as its own webpack entry →build/search-blocks/store/i18n-shim.js.@wordpress/i18nmodule ID inSearch_Blocks::register_i18n_module()viawp_register_script_module(). The browser's import map now resolves@wordpress/i18nto the shim, so calls go through the samewindow.wp.i18ninstance the classicwp-i18nscript provides.wp-jp-i18n-loaderpipeline.Search_Blocks::enqueue_i18n_runtime()enqueueswp-i18nandwp-jp-i18n-loader(registered byAutomattic\Jetpack\Assets, withstate.baseUrl/state.locale/state.domainPaths['jetpack-search-pkg']already populated).src/search-blocks/store/i18n-bootstrap.jscallswp.jpI18nLoader.downloadI18n( bundlePath, 'jetpack-search-pkg', 'plugin' )from each entry that has its own translatable strings. jp-i18n-loader hashes the bundle path againststate.domainPathsto match Jetpack's per-handle .json filenames, thensetLocaleData()s the result into the sharedwp.i18nruntime — same path@automattic/i18n-loader-webpack-plugininjects into classic-script bundles.result-utils.jsbuildRatingAriaLabel— the original SEARCH-168 offender. Now calls__( '%s out of 5 stars', ... )for the no-reviews branch and_n( '%1$s out of 5 stars based on %2$d review', '%1$s out of 5 stars based on %2$d reviews', reviewCount, ... )+sprintffor the with-reviews branch (matches the source strings the editor'sedit.jsalready extracts).store/index.jscomputeResultsCountText— drops thestate.strings?.searching ?? 'Searching…'indirection in favour of native__/_n/sprintf.active-filters/view.jsactivePills— same swap on the "Remove %s" pill aria-label.build_initial_strings()PHP seed and its test assertions. Strings live in JS source now.Design choices (why this shape, not the alternatives)
A few decisions deserve a paper trail because the obvious alternatives looked simpler at first and turned out not to be:
Why
I18nLoaderPlugin: false. The standard repo path for "make@wordpress/i18nwork in a webpack bundle" is@automattic/i18n-loader-webpack-plugin, which auto-injects a runtime that callswp.jpI18nLoader.downloadI18n()whenever a chunk loads. But re-readingI18nLoaderPlugin.js:196—loop: for ( const subchunk of chunk.getAllAsyncChunks() )— that auto-injection only fires on async sub-chunks. Our IAPI bundles are 13 self-contained entry chunks with zero async children, so the plugin would skip every one of them and emit no runtime code. Even if we worked around the secondary issue (DEP throwing on@wordpress/jp-i18n-loaderin module mode), the plugin would do nothing useful for our main chunks. We'd still need a manualdownloadI18n()call. So we leave the plugin disabled and call jp-i18n-loader's exported runtime directly fromi18n-bootstrap.js— same library it would have called for us, just invoked from the right place for our chunk shape.Why externalize via
'module @wordpress/i18n'+ a registered shim, not'var wp.i18n'. Mapping@wordpress/i18nto avar wp.i18nglobal read is what DEP does in classic-script mode by default, and at first glance it would let us drop the shim file and theregister_i18n_module()PHP. But in module-output mode DEP records the externalized value (not the original request) in itsexternalizedDepsset — sowp.i18nends up in the .asset.phpdependenciesarray. WP's script-module registry then silently refuses to enqueue any view bundle whose declared deps include an unresolvable script-module ID, breaking every block that imports@wordpress/i18n(verified empirically — search-results.js disappears from the page; flipping back to'module @wordpress/i18n'restores it). We could post-process .asset.php files to stripwp.i18n, but at that point the shim is the cleaner answer: one ESM file + onewp_register_script_module()call, no asset-rewriting hack.Why not seed strings from PHP via
wp_interactivity_state()/state.strings.*(the path RSM-267 established). It works, but it forks every translatable JS string into two source-of-truth sites — a_n()call in PHP that produces the seeded value plus a JS reader that consumes it — and the seeding has to be re-done for every new string in every new IAPI block. Routing through@wordpress/i18ncollapses both halves: the JS source carries the translatable string, the .pot extractor sees it, jp-i18n-loader fetches the translation. This PR also migrates the existingstate.strings.*consumers (computeResultsCountText,activePills.ariaLabel) so the package converges on a single i18n path.Why not bundle
@wordpress/i18ninline in each view bundle. Webpack will happily inline the package's source ifrequestToExternalModulereturnsfalse, giving each bundle its own self-contained Tannin runtime. But the bundled instance is separate fromwindow.wp.i18n—wp.jpI18nLoader.downloadI18n()writes to the classic script's instance, so translations would never reach the bundled copies. We'd have to also bundle a private jp-i18n-loader and copy state across (~+15 KB per view bundle, x13 bundles) just to get back the integration the shim gives us in 30 lines.Related product discussion/links
state.strings.*workaround)Does this pull request change what data or activity we track or use?
No.
Testing instructions
cd projects/packages/search && pnpm run test-scripts— 429/429 JS tests pass.cd projects/packages/search && composer phpunit -- --filter=Search_Blocks_Test— 18/18 PHP tests pass.add_filter( 'jetpack_search_blocks_enabled', '__return_true' )):/?s=...(or any page that renders a Jetpack Search 3.0 block).@wordpress/i18nto the shim:<script>tags includewp-i18nandwp-jp-i18n-loaderas classic scripts.wp.jpI18nLoader.downloadI18n()should fetch<lang>/plugins/<plugin-slug>-jetpack-search-pkg-<locale>-<hash>.jsonfor each search-blocks bundle and inlinesetLocaleData(...)intowp.i18nonce it lands. (en_US short-circuits in the loader — no .json fetch is expected.)search-resultsblock to theproductlayout and inspect a card's rating row: thearia-labelshould now be the translated "X out of 5 stars based on N reviews" string (front end matches editor preview).