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. |
dd7ccc7 to
8610eb4
Compare
patches @wordpress/build@0.11.0 to add packageSources config for cross-directory package discovery. mirrors upstream Gutenberg PR WordPress/gutenberg#77226 1:1. flags @automattic/number-formatters as a script-module exporter via wpScriptModuleExports so consumers can externalize it.
configures @automattic/number-formatters as a named packageSource and demonstrates both import modes in the dashboard route: static import for formatNumber, dynamic import for formatNumberCompact via React.lazy and Suspense.
25c7409 to
1b3fb87
Compare
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. |
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).