Repository navigation
wp-build dashboards: translate JavaScript strings (load catalogs + stamp text domains) - #50762
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?
|
github-actions
Bot
added
[Feature] Contact Form
[Feature] Publicize
|
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! Premium Analytics 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. |
Code Coverage SummaryCoverage changed in 13 files. Only the first 5 are listed here.
9 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
LiamSarsfield
left a comment
LiamSarsfield
left a comment
There was a problem hiding this comment.
Thanks for addressing this 🚀
-
Standalone Premium Analytics will fatal on load. Its composer.json has no plugin slug (
wp-plugin-slug/beta-plugin-slug), so the composer plugin dropsjetpack_vendor/i18n-map.phpat build, then the new bootstrap does a barerequireon that path throughalias_textdomains_from_file()and dies with "Failed opening required." Every other plugin that calls it (backup, social, starter-plugin) sets a slug so the map generates, so adding"beta-plugin-slug": "jetpack-premium-analytics"(orwp-plugin-slug, whichever channel it ships on) should sort it. I checked, and the built plugin has no map where the others do. -
PA misses catalogs for its non-content bundles. The init list only loads
routes/*/content.js, but the lazy-loaded widget renders carry strings too (e.g.Published %sin latest-post), and they're keyed on their own bundle paths, so they'll render in English. Probably want to derive the list from the emitted assets instead of hand-maintaining it. -
Catalog load can wedge first render. Every dashboard awaits the downloads before boot, and
downloadI18nhas no fetch timeout, so a stalled endpoint blocks first paint instead of falling back to English. A bounded race fixes it.
Rest looked solid.
LiamSarsfield
left a comment
LiamSarsfield
left a comment
There was a problem hiding this comment.
The manifest and the bounded wait both look right. That covers what I raised last time.
Four more things that appear to have substance:
-
The self-alias guards undo the Premium Analytics fix from earlier in this same PR.
alias_textdomain()returns at the$from === $tocheck before it populates$domain_map, andgenerateManifest()drops the package from the map outright. But that record is also the only thing feedingdomainPaths, which is the prefixdownloadI18nhashes. So standalone PA hashesbuild/routes/.../content.jsrather thanjetpack_vendor/automattic/jetpack-premium-analytics/build/..., every catalog 404s, and the loader silences exactly that error class. The guards are right, the recursion is real. It's that the map entry is carrying two contracts at once. Keeping the(ver, path)record and skipping only the filter registration should do it, though the newAssetsTestcase would need updating with it. Renaming the package domain tojetpack-premium-analytics-pkg, like backup/videopress/search, avoids the collision entirely. -
Stamping doesn't change the asset version. wp-build hashes the emitted JS into
'version'in*.asset.phpduring the esbuild pass, andstamp-textdomainsrewrites the file afterwards without recomputing it. On a bundle where the stamp is the only change, the version comes out identical to the previous release, so browsers keep serving the cached unstamped copy. Either stamp before the hash is taken, or rewrite the sibling asset file. -
The stamper matches any member call named
__,_x,_nor_nx. Inbabel-plugin-replace-textdomain, a MemberExpression whose property is infunctionsskips the import-provenance check; only bare identifiers get resolved back to@wordpress/i18n. Socache.__( 'key' )comes out ascache.__( 'key', 'jetpack-forms' ). Nothing in the tree collides today, but this is the first time it's been pointed at whole esbuild output including bundled deps, so the exposure is a lot wider than the webpack usage it was written for. -
startsWith( 'HTTP request failed:' )swallows 500s and 403s, not just the missing-catalog 404 it's meant for. That's what would keep 1 and 2 invisible in the wild. Worth reading the status and staying quiet only on 404.
Minor: PA's build-production is the only one of the seven without a clean step. And the lockfile diff is about 5k lines of tarball: metadata with no actual dependency change, so probably worth regenerating before it starts conflicting with everything else in flight.
…rom elsewhere A member call is matched by its property name alone, so `cache.__( key )` in a bundled dependency reads as gettext. That was tolerable while the plugin only ran over hand-written source; pointing it at whole esbuild output — bundled dependencies included — widens the exposure a lot, in both the stamp pass and the i18n stub the .pot is extracted from. New opt-in `requireI18nSource` traces a callee back through its binding and skips the call when it resolves to something other than `@wordpress/i18n`. It rejects what is proven foreign rather than accepting only what is proven i18n, so an unrecognised bundler shape keeps stamping instead of silently shipping a bundle in English. The trace follows esbuild's `__toESM` / `__commonJS` chain by looking for the module name or a `wp.i18n` read, both of which survive minification; the wp-build fixture now carries a decoy call that must come through unstamped and unextracted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ewrites wp-build hashes each emitted bundle into `'version'` in its sibling `*.asset.php` during the esbuild pass, and the stamp pass rewrites the bundle afterwards. On a release where the text-domain stamp is a bundle's only change, the version comes out identical to the previous one and browsers keep serving the cached, unstamped copy — the strings stay English until something else in that bundle happens to change. Recompute the version from the stamped bytes using wp-build's own scheme (sha256, first 20 hex chars). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`startsWith( 'HTTP request failed:' )` swallowed every HTTP failure, not just the missing-catalog 404 it was meant for. A 403 from a hardened languages directory or a 500 from the origin rendered the UI in English with nothing in the console — the same invisible failure the rest of this helper's diagnostics exist to prevent. Read the status back out of the message and keep quiet only on 404. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`build-production` was the only one of the seven dashboards that never cleaned build/ — it inlined the build steps instead of going through `build`, which is where the other packages get their clean. Route it through `build` like they do, and add the `clean` script it lacked. The lockfile also carried ~2.5k added `tarball:` metadata lines with no dependency change behind them; regenerated with `--resolution-only` so only the patch hash and the new workspace `init` packages remain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`downloadI18n()` prepends a domain's prefix to every bundle path looked up under it, so a domain can only carry one package's path. That was implicit while every entry was a package domain distinct from its plugin's; now that a plugin's own domain can hold one, a plugin that both bundled such a package and loaded its own bundles under that domain would send its own catalogs looking under the package's path. Nothing does that today, and the fix if anything ever needs to is the existing `-pkg` textdomain convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows #50850, which moved the package to `jetpack-premium-analytics-pkg` so it no longer collides with the plugin slug. The build-time stamp reads the domain from composer.json and picked the new one up on its own; this is the runtime half, where the domain is passed explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0af5eee to
a7a40ae
Compare
github-actions
Bot
removed
the
[Status] Needs Review
|
👋 @dhasilva Heads-up on something we ran into testing Premium Analytics dashboard on a non-English locale. The blocking split is fine (~16 awaited). It's the ~84 widget catalogs: unawaited, but still all requested at boot for widgets that may never render. Wondering about loading each at its own dynamic import instead — did you already look at that? |
…t-domain stamping) (#51086) * Build debug code frames only when debug output is enabled `buildCodeFrameError()` renders an excerpt of the surrounding source, so its cost scales with the size of the file. Passing it straight to `debug()` reads as free when debug output is off, but JavaScript evaluates call arguments eagerly, so the frame was built for every matched call either way. That was cheap while this plugin ran over hand-written source with domains already present. Since #50762 it also runs over the esbuild bundles that `stamp-textdomains` post-processes, where the domain is absent by construction — so the missing-domain branch, the one that builds a frame, became the hot path on multi-megabyte files. Measured over premium-analytics' 342 built bundles: 211.8s -> 6.2s (34x), with byte-identical output for every file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDM1RKuvDmbfyMZspnghaa * Shorten debug code frame comment * Add hyphens before @PARAM descriptions Fixes the two jsdoc/require-hyphen-before-param-description warnings that failed the lint check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDM1RKuvDmbfyMZspnghaa --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
| * would be applied to the plugin's catalogs too, and they would all 404. No | ||
| * plugin does both today. One that needs to should give the package a | ||
| * distinct textdomain, the way `jetpack-backup-pkg` and | ||
| * `jetpack-videopress-pkg` do. |
There was a problem hiding this comment.
Did something actually run into all this, or is it that someone's AI didn't realize "doing that is already broken, plus .github/files/lint-project-structure.sh will fail CI if a plugin and a package share textdomains in the first place" and no one questioned it?
| // A self-alias would make filter_gettext() re-translate into the same | ||
| // domain, recursing infinitely on any untranslated string (a package | ||
| // textdomain can collide with its containing plugin's slug). | ||
| if ( $from === $to ) { |
There was a problem hiding this comment.
Seems like a legitimate bug, but more likely it should just throw so people actually fix it.
That should also allow for reverting most of the other complication added in here.
| // untranslated string in it. The path is still needed — it is | ||
| // the prefix WordPress hashes to name the package's JavaScript | ||
| // translation files — so it goes in `paths`, which registers | ||
| // only that. |
There was a problem hiding this comment.
Same thing here too, did anything actually run into this or is it AI lacking context and getting confused?
Again, just throwing so people actually fix the error would probably make more sense than adding a bunch of complexity to work around a case CI already rejects.
PR #50762 caused a significant rise in build times, which was fixed by #51086. Let's turn the build times back down after the fix. This effectively reverts #51029. As for #51059, the progression is 25 before → 40 during the rise → 30 now, to leave some leeway since full builds are up to ~18 minutes now.
Every item confirmed against trunk before it was written in. Corrections: - The restore does not protect `pagenow`. `admin-header.php` prints it from `$current_screen->id` before it fires `admin_enqueue_scripts`, so only the old `current_screen` alias ever reached it. Moved to the old-pattern line. - Seven ports shipped without the restore, not five. #52471 also fixed Podcast and SEO. - The JITM container belongs in the page shell, not in a tab panel. `Tabs.Panel` defaults to `keepMounted={ false }`, so Search's container leaves the DOM on a tab switch. Cite Social and Boost instead. - An empty `apiRoot` breaks `@automattic/jetpack-api`, not every API call. `@wordpress/api-fetch` has its own root. - #50762 fixed seven dashboards. SEO, Podcast and Scan still have no `packages/init`, no `build:stamp-textdomains` and no i18n loader, so §6 now says to copy Backup or Search rather than the SEO file it cites. - §8 required a visible error and then called a blank page expected. Split into two rules: a brand-new route shows an error, a retired port renders nothing and that is accepted. §9 step 7 matches. - Drop the port count and list, which was already stale. Name the flag mechanism: a `Feature_Flags` flag, which is where ports should land even though the shipped ones still use bare filters. Additions, each one something a port needs and the checklist omitted: the polyfills package in both manifests; the `.gitattributes` excludes and the `build/**` include; importing `route.scss` from `stage.tsx`, since `@wordpress/build` records `hasStyle` and never reads it; a shared mixin for multi-route layout; the Safe Mode banner, hidden by the same template rule; and moving the legacy entry's side effects into `stage.tsx`. Linear links replaced with bare issue IDs — external contributors cannot open them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add the wp-build porting checklist Nine dashboards have shipped through wp-build so far, and the same traps kept recurring: the screen-ID alias shipping without its restore (five ports), JITM views logged for cards nobody could see, wp_localize_script silently doing nothing against an ES module page, and a build/ fallback that disagreed between call sites. Fold the checklist that already existed piecemeal across four Linear comments on JETPACK-2575, plus JETPACK-2573's verification steps and JETPACK-2687/2689's screen-ID findings, into one document in docs/ so the next port can follow it without rediscovering any of this. Fixes JETPACK-2575 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Checklist: unwrap, fix the dashboard count and two section cross-references * Checklist: correct the boot-bump attribution and two fix-vs-regression citations * Checklist: fix the §8 attribution and link the doc from monorepo.md `is_wp_build_dashboard_active()` lives in `Jetpack_Backup`, not in Search. Search had it while #52416 was current; #52506 deleted it with the legacy bundle, which §8 already cites. Attribute the shared-predicate pattern to Backup, and describe Search's post-retirement gates as they are now. Drop the duplicate symbol+line citation in the second bullet, and add a pointer from `docs/monorepo.md` § Building so the checklist is reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Checklist: apply the review Every item confirmed against trunk before it was written in. Corrections: - The restore does not protect `pagenow`. `admin-header.php` prints it from `$current_screen->id` before it fires `admin_enqueue_scripts`, so only the old `current_screen` alias ever reached it. Moved to the old-pattern line. - Seven ports shipped without the restore, not five. #52471 also fixed Podcast and SEO. - The JITM container belongs in the page shell, not in a tab panel. `Tabs.Panel` defaults to `keepMounted={ false }`, so Search's container leaves the DOM on a tab switch. Cite Social and Boost instead. - An empty `apiRoot` breaks `@automattic/jetpack-api`, not every API call. `@wordpress/api-fetch` has its own root. - #50762 fixed seven dashboards. SEO, Podcast and Scan still have no `packages/init`, no `build:stamp-textdomains` and no i18n loader, so §6 now says to copy Backup or Search rather than the SEO file it cites. - §8 required a visible error and then called a blank page expected. Split into two rules: a brand-new route shows an error, a retired port renders nothing and that is accepted. §9 step 7 matches. - Drop the port count and list, which was already stale. Name the flag mechanism: a `Feature_Flags` flag, which is where ports should land even though the shipped ones still use bare filters. Additions, each one something a port needs and the checklist omitted: the polyfills package in both manifests; the `.gitattributes` excludes and the `build/**` include; importing `route.scss` from `stage.tsx`, since `@wordpress/build` records `hasStyle` and never reads it; a shared mixin for multi-route layout; the Safe Mode banner, hidden by the same template rule; and moving the legacy entry's side effects into `stage.tsx`. Linear links replaced with bare issue IDs — external contributors cannot open them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
The wp-build Podcast dashboard shipped with its strings in English on non-English sites: nothing downloads its JS translation catalog at boot. This applies the same approach #50762 used for the other wp-build dashboards. - Add a boot `init` package that calls loadI18nCatalogs( 'jetpack-podcast' ). - Register it on the dashboard page and stamp the text domain / write the i18n manifest after wp-build. - Enqueue wp-jp-i18n-loader on the Podcast page. - Register the init script module directly when `wp_default_scripts` has already fired. The Podcast build is loaded on admin_menu, after that hook, so the generated registrar never ran and the init module was missing from the import map ("Failed to resolve module specifier"). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The wp-build Podcast dashboard shipped with its strings in English on non-English sites: nothing downloads its JS translation catalog at boot. This applies the same approach #50762 used for the other wp-build dashboards. - Add a boot `init` package that calls loadI18nCatalogs( 'jetpack-podcast' ). - Register it on the dashboard page and stamp the text domain / write the i18n manifest after wp-build. - Enqueue wp-jp-i18n-loader on the Podcast page. - Register the init script module directly when `wp_default_scripts` has already fired. The Podcast build is loaded on admin_menu, after that hook, so the generated registrar never ran and the init module was missing from the import map ("Failed to resolve module specifier"). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The wp-build Podcast dashboard shipped with its strings in English on non-English sites: nothing downloads its JS translation catalog at boot. This applies the same approach #50762 used for the other wp-build dashboards. - Add a boot `init` package that calls loadI18nCatalogs( 'jetpack-podcast' ). - Register it on the dashboard page and stamp the text domain / write the i18n manifest after wp-build. - Enqueue wp-jp-i18n-loader on the Podcast page. - Register the init script module directly when `wp_default_scripts` has already fired. The Podcast build is loaded on admin_menu, after that hook, so the generated registrar never ran and the init module was missing from the import map ("Failed to resolve module specifier"). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
* Podcast: load the dashboard's JavaScript translations The wp-build Podcast dashboard shipped with its strings in English on non-English sites: nothing downloads its JS translation catalog at boot. This applies the same approach #50762 used for the other wp-build dashboards. - Add a boot `init` package that calls loadI18nCatalogs( 'jetpack-podcast' ). - Register it on the dashboard page and stamp the text domain / write the i18n manifest after wp-build. - Enqueue wp-jp-i18n-loader on the Podcast page. - Register the init script module directly when `wp_default_scripts` has already fired. The Podcast build is loaded on admin_menu, after that hook, so the generated registrar never ran and the init module was missing from the import map ("Failed to resolve module specifier"). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Podcast: suppress the Phan undeclared-function error for the generated registrar jetpack_podcast_register_script_modules() is defined by the generated build/modules.php, which Phan doesn't analyze. The call is already guarded by function_exists(). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Podcast: cover the translation loader changes with tests Moves the late script-module registration into a small method that takes the registrar as a parameter, so it can be tested without the generated build code (and no longer needs a Phan suppression). Adds tests for the loader enqueue and the registration. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Add changelog entry for plugins/jetpack * Refactor to follow the pattern from #50762 for consistency --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Daniel Lind <daniel.lind@automattic.com>
5 participants
Fixes # N/A
Proposed changes
JavaScript strings on the modernized wp-build (esbuild) dashboards shipped mostly untranslated: the pipeline externalizes
@wordpress/i18ntowindow.wp.i18nbut never loads the translation catalog, and it drops the package text domain from the built__()/_x()/_n()calls. Only ~26% of strings (the ones that happened to keep the right domain) translated — the DataViews /@wordpress/uichrome (table headers, filters, pagination, search) stayed English regardless of site locale.This fixes both root causes for seven dashboards (activity-log, publicize/Social, videopress, backup, newsletter, forms, premium-analytics), generalizing the approach shipped for Activity Log in #50714.
packages/wp-build-polyfills:stamp-textdomains— a post-build CLI that runs@automattic/babel-plugin-replace-textdomainover the esbuild output, stamping the package text domain onto every gettext call inbuild/{routes,scripts,modules,widgets}(both.jsand.min.js). Idempotent; unit-tested.loadI18nCatalogs( domain, bundles )— a shared init-module helper that downloads and installs the JS catalogs viawp.jpI18nLoaderbefore routes render.strip-unminified-prodnow replaces a string-bearing unminified.jswith a generated i18n reference stub (only its gettext calls + translator comments) instead of deleting it. GlotPress ignores*.min.js(see anomiex's comment), so deleting the.jswould leave production builds with nothing to extract strings from and no catalogs would ever be generated at release. Same path →make-potreferences and WordPress'md5( path )-keyed catalog names keep matching; never loaded at runtime (the generated PHP loaders stay pinned to.min.js). String-less.js,.css, and sourcemaps are still deleted.initsub-package that calls the helper,wpPlugin.pagesin object form, the stamp step chained afterbuild:wp-build, awp-jp-i18n-loaderenqueue on the dashboard page, and a directjetpack_<name>_register_script_modules()call (these load wp-build atadmin_menu, afterwp_default_scriptshas fired). Forms is the exception — it loads wp-build at plugins-load time, so the generated registration hook lands in time and no direct call is needed (verified the init module reaches the page's import map).alias_textdomains_from_file()call to the standalone Premium Analytics plugin (the social/videopress/backup/jetpack plugins already had it) so the loader computes the right catalog URLs there.Related product discussion/links
@wordpress/buildeffort (it has no build-time domain-stamping hook — verified inlib/build.mjs): Slack #jetpack-monorepo threadp1774956551.856089(jsnajdr). This PR'sstamp-textdomainstool could feed that.seo,podcast,scan, andjetpack-mu-wpcomalso build with wp-build and need the same treatment.wp_set_script_module_translations()(wordpress-develop#11543) may eventually replace both halves of this once Jetpack's WP floor reaches 7.0 (noted in code comments).Does this pull request change what data or activity we track or use?
No.
Testing instructions
In the Jetpack Docker environment:
jetpack build packages/{wp-build-polyfills,activity-log,publicize,videopress,backup,newsletter,forms,premium-analytics}.wp-build --watchrebuilds route bundles without thestamp-textdomainsstep, so watch output is unstamped and will look untranslated on a non-en_USsite — that's expected, not a regression.tools/docker/bin/fakepot.sh <monorepo>/projects/plugins/jetpack(and also.../projects/plugins/backupif the standalone Backup plugin is active — its dashboard resolves catalogs under thejetpack-backupdomain).@wordpress/uibuttons/labels) now renders pig-latin:admin.php?page=jetpack-activity-logadmin.php?page=jetpack-socialadmin.php?page=jetpack-videopressadmin.php?page=jetpack-backup(modernized dashboard is behindrsm_jetpack_ui_modernization_backup)admin.php?page=jetpack-newsletteradmin.php?page=jetpack-forms-responses-wp-admin🤖 Generated with Claude Code