Bridge @wordpress/build to upstream identity + discovery (#78822, #77465)#48089
Bridge @wordpress/build to upstream identity + discovery (#78822, #77465)#48089retrofox wants to merge 11 commits into
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 🤖 🔴 Action required: Please include detailed testing steps, explaining how to test your change, like so: 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so: 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 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. 🤷 |
There was a problem hiding this comment.
Move to the existing .pnpm-patches/ directory.
Also, I'm not terribly fond of the "apply a large patch I hope to get merged upstream soon" approach here; such hopes seem frequently dashed.
There was a problem hiding this comment.
Yeah, that's fair. We need to try to move these changes upstream asap.
There was a problem hiding this comment.
Unneeded. Any comments about a patch can be included at the top of the patch file itself, as shown by the existing patches.
|
@anomiex PR is not ready for review. Sorry about it. We need to land the package and plugin PRs first |
|
Will this idea cause problems if multiple different wp-build-using plugins each include incompatible versions of It looks like, unless all plugins are careful to only load their |
6f55a70 to
ae38a48
Compare
a53fd17 to
7ccaa1a
Compare
Great point. As far as I understand, this is inherent to WordPress's global script module registry, not specific to The same trade-off already exists with Looking at the generated Within the Jetpack monorepo, this is mitigated by the shared lockfile: all plugins built from the same commit get the same version of Without This is a crucial point to keep in mind as more plugins adopt this pattern. Does it make sense? |
|
Something to keep in mind is that it doesn't seem like the people working on wp-build are considering these sorts of edge cases; they seem pretty focused on Gutenberg and Core. You should probably copy that information to your WordPress/gutenberg#77226 PR. |
|
Note to also keep testing Forms builds and works well with this, as it's the only other thing using WP Build: |
2eafa61 to
ecf8b90
Compare
7ccaa1a to
e9b7a96
Compare
|
Addressed all three:
|
Good idea. I'll add the script module collision analysis to the Gutenberg PR #77226 so upstream maintainers are aware of the deduplication trade-off. |
Confirmed. |
regenerate patch against 0.12.0 sources using upstream PR #77226 diff. header embeds the version-agnostic rebase procedure.
# Conflicts: # pnpm-lock.yaml # projects/packages/premium-analytics/package.json
Replaces the @wordpress/build@0.12.0 packageSources patch with a 0.13.0 patch that mirrors the new upstream proposal: identity from package.json#name, discovery via convention (dependencies that declare wpScriptModuleExports), plus a node_modules walk-up fallback in getPackageInfo for packages whose exports field hides ./package.json. No more wpPlugin.packageSources config needed on the consumer side.
mirror upstream PR #77465 so shared non-core modules use Core first-wins
rename init to @automattic/jetpack-premium-analytics-init; align pages[].init so name, specifier, and module ID match
regenerate from #78822 + #77465 diffs: drop the node_modules walk-up (number-formatters now exposes ./package.json) and adopt #77465's exact template comments
lets wp-build convention discovery read the manifest without a resolver workaround
trunk bumped @wordpress/build to 0.14.0 monorepo-wide; patch regenerated from #78822 + #77465 against 0.14.0
|
Re-validated end-to-end on the current branch (
On the "last plugin wins on version skew" concern. That's now handled by the companion #77465. Stock if ( ! isset( $this->registered[ $id ] ) ) {
// ...register...
}i.e. first registration wins, deterministically. The same model Within the monorepo, the shared lockfile pins one version, so the copies are identical anyway; cross-release skew falls back to first-wins instead of an arbitrary last-wins clobber. Note the approach also pivoted away from the |
I note that just changes it to "first plugin wins"; the concern itself about multiple plugins with incompatible versions of the module still exists.
That assumes that everyone has matching versions of multiple plugins, and no external plugin starts using the package. Neither seem like things to rely on. |
Defer the internal-packages naming docs until the upstream wp-build identity change (gutenberg#78822 / #48089) lands; restore README to trunk.
You're right, and I won't pretend otherwise: #77465 turns last-plugin-wins into first-plugin-wins, but it doesn't make version skew go away. Being upfront: this is the same concern @youknowriad raised on the upstream PR (#77226), "separate plugins shouldn't register the same module names; it creates conflicts if they require different versions." This patch is three independent pieces:
So there are two honest framings:
My proposal: treat this patch as a bridge scoped to the monorepo, where it's safe today, and drive the compatibility-aware ID upstream before this becomes something other plugins (or external ones) rely on. If you'd rather not carry the convention-discovery piece until that lands, I can split it out and keep only name-based identity plus the deregister scope: that still gives us the dependency-confusion fix with none of the shared-ID risk. WTDT? |
|
FYI it'll be important to test with Gutenberg, with WP 7.0 and with WP 6.9, as we've had version compatibility issues in the past with WP Build (hence polyfills). |
|
As I said earlier, I'm wary of an "apply a large patch I hope to get merged upstream soon" approach, as such hopes seem frequently dashed. Whichever piece it is. |
|
I agree. I'm still working on it and would like to share it today or tomorrow. Clearly, it's not my area, if I have one. Thanks, and sorry for the noise. |
#49189) * feat(premium-analytics): add tsconfig paths and typecheck for internal packages * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * docs(premium-analytics): clarify internal-package naming and rename init Address PR review feedback on the "Internal packages" section: - Lead with scope intent: internal-only, never published, in-tree symlink-only resolution (answers the npm-squatting concern) - Explicitly explain the structural dual naming between the package name field and the wp-build-derived import specifier - Rename packages/init from `_@jetpack-premium-analytics/init` to `@automattic/jetpack-premium-analytics-init` so the codebase matches the documented pattern (the old placeholder is invalid to pnpm) * docs(premium-analytics): revert internal-packages README section Defer the internal-packages naming docs until the upstream wp-build identity change (gutenberg#78822 / #48089) lands; restore README to trunk. * fix(premium-analytics): align route name with internal-package convention Rename routes/dashboard to @automattic/jetpack-premium-analytics-dashboard-route to match the packages/* naming (per review), and drop the stale changelog line about README build docs that were reverted. Build output is unchanged (routes key off the directory name). * docs(premium-analytics): fix stale README reference in tsconfig comment --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…lytics (#49224) * feat(premium-analytics): add tsconfig paths and typecheck for internal packages * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * docs(premium-analytics): clarify internal-package naming and rename init Address PR review feedback on the "Internal packages" section: - Lead with scope intent: internal-only, never published, in-tree symlink-only resolution (answers the npm-squatting concern) - Explicitly explain the structural dual naming between the package name field and the wp-build-derived import specifier - Rename packages/init from `_@jetpack-premium-analytics/init` to `@automattic/jetpack-premium-analytics-init` so the codebase matches the documented pattern (the old placeholder is invalid to pnpm) * chore(premium-analytics): add @wordpress/primitives dependency Required by the icons package being ported in subsequent commits. Refs WOOA7S-1314 * feat(premium-analytics): copy icons sources from next-woocommerce-analytics Verbatim port of the 13 illustrated WPDS icons (calendar, channel, coupon, customer, device, goal, location, megaphone, payment, payment-return, product-blouse, reports, search) plus the barrel index that re-exports navigation/settings/plus/info from @wordpress/icons. The upstream stories file is dropped (depends on @wordpress/ui — a CIAB-only package). The upstream leaf tsconfig.json is also dropped: this package's parent tsconfig already covers packages/**/*. The leaf package.json is added in a follow-up commit (adapted for the monorepo, not copyable verbatim). Refs WOOA7S-1314 * feat(premium-analytics): add icons package manifest Adapt upstream package.json for the Jetpack monorepo: - Rename to @automattic/jetpack-premium-analytics-icons (the import specifier @jetpack-premium-analytics/icons comes from the parent's wpPlugin.packageNamespace; see parent README). - private: true and version 0.1.0 (matches sibling packages/init). - Pin @wordpress/primitives 4.46.0 (Jetpack convention) and bump @wordpress/icons to ^13.0.0 (matches parent). - Add main/types pointing at src/index.ts and sideEffects: false so tooling can resolve the package from source. - Drop upstream wpModule/exports (CIAB-specific build conventions) and the design-system / @wordpress/ui devDeps (only used by the now-dropped stories file). Refs WOOA7S-1314 * docs(premium-analytics): add icons README The upstream icons package shipped no README; document the exported icons, the WPDS / @wordpress/icons split, and the dual-naming convention link back to the parent README. Refs WOOA7S-1314 * changelog: add entry for premium-analytics icons port Refs WOOA7S-1314 * feat(premium-analytics): restore icons stories with Jetpack adaptations @wordpress/ui is a published package (already pinned at 0.13.0 by sibling packages videopress and publicize), not CIAB-only — restore the upstream stories gallery so the icons remain discoverable. Adaptations from upstream: - Title: 'Icons/Gallery' → 'Packages/Premium Analytics/Icons' (Jetpack story-tree convention). - Add component: Icon to the default export to satisfy storybook/csf-component. - Drop the External/Internal-dependencies comment separators and the blank line between import groups so import/order passes. Adds @wordpress/ui@0.13.0 and @storybook/react@10.3.6 as devDeps on both the parent and the leaf package.json (parent so pnpm resolves them; leaf so import/no-extraneous-dependencies finds them). Note: the stories file is not yet wired into the central Storybook config at projects/js-packages/storybook/storybook/projects.js, so it won't render in Storybook until that list is extended. Tracked separately. Refs WOOA7S-1314 * chore(premium-analytics): pin @storybook/react peer deps Adding @storybook/react@10.3.6 as a devDep brings unmet peer dependencies (storybook, typescript, @testing-library/dom) that ERR_PNPM_PEER_DEP_ISSUES blocks fresh CI installs on. Pin the same versions sibling packages videopress and publicize already pin: storybook@10.3.6 typescript@5.9.3 @testing-library/dom@10.4.1 Refs WOOA7S-1314 * refactor(premium-analytics): drop @wordpress/icons re-exports from icons package * docs(premium-analytics): revert internal-packages README section Defer the internal-packages naming docs until the upstream wp-build identity change (gutenberg#78822 / #48089) lands; restore README to trunk. * fix(premium-analytics): align route name with internal-package convention Rename routes/dashboard to @automattic/jetpack-premium-analytics-dashboard-route to match the packages/* naming (per review), and drop the stale changelog line about README build docs that were reverted. Build output is unchanged (routes key off the directory name). * docs(premium-analytics): use canonical package name in icons README * docs(premium-analytics): fix icons README import example and viewBox claims * docs(premium-analytics): drop per-icon size details from icons README * feat(premium-analytics): integrate icons stories into shared Storybook --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…analytics (#49221) * feat(premium-analytics): add tsconfig paths and typecheck for internal packages * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * docs(premium-analytics): clarify internal-package naming and rename init Address PR review feedback on the "Internal packages" section: - Lead with scope intent: internal-only, never published, in-tree symlink-only resolution (answers the npm-squatting concern) - Explicitly explain the structural dual naming between the package name field and the wp-build-derived import specifier - Rename packages/init from `_@jetpack-premium-analytics/init` to `@automattic/jetpack-premium-analytics-init` so the codebase matches the documented pattern (the old placeholder is invalid to pnpm) * feat(premium-analytics): port datetime package from next-woocommerce-analytics Near-verbatim copy of next-woocommerce-analytics/packages/datetime/. Provides timezone-aware date helpers (date-fns + @date-fns/tz) and date-range presets used by analytics widgets. Monorepo adaptations: - Rename to @automattic/jetpack-premium-analytics-datetime per the internal-packages convention (parent README, "Internal packages"). - Pin date-fns 4.1.0 and @date-fns/tz 1.4.1 instead of upstream "*"; add @wordpress/i18n ^6.9.0 to match parent. - Drop the leaf tsconfig.json — parent already includes packages/**/* and packages/init has no leaf tsconfig either. - Replace `any[]` with `unknown[]` in GrowTuple constraint (tz.ts). - Add date-fns + @date-fns/tz to parent package.json so imports resolve from premium-analytics/node_modules (packages/datetime is not a pnpm workspace member, only the parent is). - ESLint text-domain autofix swept `woocommerce-analytics` → `jetpack-premium-analytics` in preset labels. - New eslint.config.mjs softens JSDoc rules for packages/datetime/** to keep upstream re-syncs mechanical (mirrors activity-log's DateRangePicker precedent). Refs WOOA7S-1312 * changelog: add entry for premium-analytics datetime port Refs WOOA7S-1312 * docs(premium-analytics): rewrite datetime README header for monorepo context - Title matches the import specifier consumers actually type. - Description swaps WooCommerce Analytics → Jetpack Premium Analytics. - Notes it's an internal (non-published) package and points to the parent README for the dual-naming convention. Formatter normalized list bullet spacing and one type-union wrap as a side-effect of saving the file. Refs WOOA7S-1312 * chore(premium-analytics): reframe datetime eslint override as temporary Drop the upstream-path reference; note that backfilling JSDoc on the ported helpers should let us remove this whole config file. Refs WOOA7S-1312 * docs(premium-analytics): drop internal-package note from datetime README Refs WOOA7S-1312 * docs(premium-analytics): revert internal-packages README section Defer the internal-packages naming docs until the upstream wp-build identity change (gutenberg#78822 / #48089) lands; restore README to trunk. * fix(premium-analytics): align route name with internal-package convention Rename routes/dashboard to @automattic/jetpack-premium-analytics-dashboard-route to match the packages/* naming (per review), and drop the stale changelog line about README build docs that were reverted. Build output is unchanged (routes key off the directory name). * docs(premium-analytics): use canonical package name in datetime README heading * Update projects/packages/premium-analytics/packages/datetime/README.md Co-authored-by: Dognose <dognose24@users.noreply.github.com> * Update projects/packages/premium-analytics/packages/datetime/src/tz.ts Co-authored-by: Dognose <dognose24@users.noreply.github.com> * Update projects/packages/premium-analytics/packages/datetime/src/tz.ts Co-authored-by: Dognose <dognose24@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Dognose <dognose24@users.noreply.github.com>
…e-analytics (#49226) * feat(premium-analytics): add tsconfig paths and typecheck for internal packages * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * docs(premium-analytics): clarify internal-package naming and rename init Address PR review feedback on the "Internal packages" section: - Lead with scope intent: internal-only, never published, in-tree symlink-only resolution (answers the npm-squatting concern) - Explicitly explain the structural dual naming between the package name field and the wp-build-derived import specifier - Rename packages/init from `_@jetpack-premium-analytics/init` to `@automattic/jetpack-premium-analytics-init` so the codebase matches the documented pattern (the old placeholder is invalid to pnpm) * chore(premium-analytics): copy formatters package from next-woocommerce-analytics * chore(premium-analytics): adapt formatters package.json for internal-package convention * chore(premium-analytics): pin formatters deps for Jetpack monorepo * docs(premium-analytics): adapt formatters README and code comments for monorepo * chore(premium-analytics): wire formatters deps into parent and relax JSDoc rules * chore(premium-analytics): format formatters and clean up lint issues * changelog(premium-analytics): add entry for WOOA7S-1313 formatters port * chore(premium-analytics): add @types/jest for formatters test typecheck * docs(premium-analytics): revert internal-packages README section Defer the internal-packages naming docs until the upstream wp-build identity change (gutenberg#78822 / #48089) lands; restore README to trunk. * fix(premium-analytics): align route name with internal-package convention Rename routes/dashboard to @automattic/jetpack-premium-analytics-dashboard-route to match the packages/* naming (per review), and drop the stale changelog line about README build docs that were reverted. Build output is unchanged (routes key off the directory name). * docs(premium-analytics): use canonical package name in formatters README * Update projects/packages/premium-analytics/packages/formatters/README.md Co-authored-by: Dognose <dognose24@users.noreply.github.com> * Update projects/packages/premium-analytics/packages/formatters/README.md Co-authored-by: Dognose <dognose24@users.noreply.github.com> * Update projects/packages/premium-analytics/packages/formatters/src/metric/format-metric-value.ts Co-authored-by: Dognose <dognose24@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Dognose <dognose24@users.noreply.github.com>
What?
Replaces the pnpm patch on
@wordpress/buildwith one that mirrors two upstream Gutenberg PRs applied to@wordpress/build@0.13.0:package.json#name, externalizes internal-package imports by exact name, and discovers script-module packages outside./packages/via convention (anydependenciesentry whosepackage.jsondeclareswpScriptModuleExports).wp_deregister_script_module()calls to@wordpress/*IDs, so shared non-core packages get Core's idempotent first-wins instead of last-plugin-wins.Plus one consumer-side change:
@automattic/number-formattersexposes./package.jsonin itsexportsso the convention discovery can read its manifest.Supersedes the earlier
wpPlugin.packageSourcesconfig approach (closed upstream as #77226 after slippery-slope feedback). No new wp-build config; the new behavior is convention-driven.wpModule/wpScriptModuleExportspackages outside./packages/*WordPress/gutenberg#77225Closes WOOA7S-1341 WOOA7S-1342
Why?
Stock
@wordpress/buildonly externalizes@<packageNamespace>/*and only scans./packages/. A shared package like@automattic/number-formattersmatches no plugin's namespace and lives injs-packages/, so it gets inlined into every consumer's bundle: duplicated code, no script-module deduplication. And because the script-module ID is derived fromwpPlugin.packageNamespace, a package's npmnameand its runtime specifier are forced to diverge, which needs atsconfigpathsalias and trips dependency-confusion scanners.The two upstream PRs fix both: identity comes from the package's own
name, and shared packages are discovered and registered once. This PR is the bridge that validates that bundle end-to-end inside Jetpack until Core ships it.How?
The patch is a verified faithful mirror of core
The patch is exactly #78822 + #77465 applied to
@wordpress/build@0.13.0. The only addition is a one-linecreateNodeRequireimport that0.13.0predates (the PR branch sits on newer trunk where it already exists). Verified by a two-tree diff: pristine0.13.0+ the two PR.diffs is byte-identical to pristine + this patch. The patch header documents the rebase recipe so it can be regenerated against a future@wordpress/buildwithout a Gutenberg checkout.premium-analytics migrated to name-based identity
With identity coming from
name, the localinitpackage is renamed to@automattic/jetpack-premium-analytics-init, andwpPlugin.pages[].initis updated to match. Result: npm name === import specifier === script-module ID, with notsconfigalias and no unregistered-scope scanner exposure.number-formatters as the shared module
@automattic/number-formattersdeclareswpScriptModuleExportsand exposes./package.json. The premium-analytics dashboard route imports it both statically (formatNumber) and dynamically (formatNumberCompact). Both resolve to a single shared@automattic/number-formattersscript module, externalized out of the route bundle.This is a monorepo-wide win, not premium-analytics only
The patch is applied to every
@wordpress/buildconsumer throughpnpm-workspace.yaml, so the deduplication happens automatically wherever a wp-build plugin already imports a sharedwpScriptModuleExportspackage. No per-consumer change is needed.@automattic/jetpack-forms(a shipping package, not experimental) is the clearest example:number-formattersis imported across 13 source files in its routes and dashboard. Its wp-build run logs✔ Bundled @automattic/number-formattersand emits a single sharedbuild/modules/@automattic/number-formatters/index.min.js(14.6 KB), byte-identical to the one premium-analytics produces. Without the patch, that formatter code is inlined into each wp-build route bundle that imports it; with the patch they share one module, registered once in WordPress.This is the real point of the change: it improves all of Jetpack's wp-build integration, not a single plugin.
Testing
Build premium-analytics and confirm the shared module is deduplicated:
build/modules/registry.phplists@automattic/number-formattersand@automattic/jetpack-premium-analytics-init.build/routes/dashboard/content.min.jsis ~1.45 KB and does not inline number-formatters. Contrast: ~16 KB with it inlined. A separatebuild/modules/@automattic/number-formatters/index.min.jsholds the single shared copy.build/modules.phpderegisters only@wordpress/*IDs (if ( str_starts_with( $module['id'], '@wordpress/' ) )).Because the patch is global, the same deduplication applies to every wp-build consumer.
@automattic/jetpack-forms, for instance, emits the identical sharedbuild/modules/@automattic/number-formatters/index.min.jsfrom its own build, with no forms-specific change.To verify the faithful-mirror claim, follow the recipe in the patch header: it rebuilds the patched tree from the two upstream
.diffs and diffs it against this patch (identical).(Optional) Live in WordPress: activate the Premium Analytics plugin alongside the Gutenberg plugin and open the dashboard. The page loads the renamed
initmodule (which sets the menu icon) and number-formatters as a shared module, in both static and dynamic form.Trade-offs
Shared script modules across plugins use the same trust model
externalNamespacesalready applies to@wordpress/*: one registration in WordPress's global registry. With #77465 a shared non-core ID falls through to Core's idempotent first-wins (deterministic) instead of last-plugin-wins. Within the monorepo the shared lockfile pins one version; cross-release version skew is the same exposurewp_register_script()has always had.Follow-ups
@wordpress/build.paths+ dual-naming README) to the parts that survive name-based identity (thetypecheckscript and theinitrename).