Build Tools: Add wpPlugin.packageSources for additional package directories#77226
Closed
retrofox wants to merge 7 commits into
Closed
Build Tools: Add wpPlugin.packageSources for additional package directories#77226retrofox wants to merge 7 commits into
retrofox wants to merge 7 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
retrofox
force-pushed
the
add/wp-build-sources-config
branch
from
3125747 to
8221198
Compare
|
Size Change: 0 B Total Size: 7.75 MB ℹ️ View Unchanged
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support in @wordpress/build for discovering buildable packages from additional roots via a new wpPlugin.sources configuration field, enabling multi-plugin monorepos to share packages outside a single plugin’s ./packages/ directory.
Changes:
- Refactors package discovery to build a registry (
Map) that caches each discovered package’s directory and parsedpackage.json. - Introduces
wpPlugin.sourcesto scan{source}/packages/*/package.jsonin addition to the local root, with local-first precedence. - Documents the new configuration option and records it in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
packages/wp-build/lib/build.mjs |
Implements multi-root package discovery + registry refactor and updates call sites to use registry entries. |
packages/wp-build/README.md |
Documents the new wpPlugin.sources configuration and its precedence rules. |
packages/wp-build/CHANGELOG.md |
Adds an unreleased enhancement entry for wpPlugin.sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
+117
| const ALL_ROOTS = [ | ||
| ROOT_DIR, | ||
| ...( WP_PLUGIN_CONFIG.sources || [] ).map( ( s ) => | ||
| path.resolve( ROOT_DIR, s ) | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
wpPlugin.sources is assumed to be an array; if it’s accidentally configured as a string/object, this will throw at runtime (.map is not a function). Consider validating sources with Array.isArray (and optionally ensuring each entry is a string) and throwing a clear configuration error, or coercing invalid values to an empty array.
Contributor
Author
There was a problem hiding this comment.
The codebase doesn't validate any wpPlugin field — pages, externalNamespaces, etc. all use || [] / || {} without Array.isArray. Keeping sources consistent with that pattern.
Comment on lines
519
to
523
| * | ||
| * @param {string} packageName Package name. | ||
| * @param {Object} options Optional bundling options. | ||
| * @return {Promise<boolean>} True if the package was bundled, false otherwise. | ||
| */ |
There was a problem hiding this comment.
The JSDoc return type for bundlePackage says it returns Promise<boolean>, but the function returns either false or an object containing { modules, scripts, styles }. Please update the return annotation to match the actual return shape so callers and tooling don’t get misleading types.
Contributor
Author
There was a problem hiding this comment.
Pre-existing — the return type was already wrong before this PR. Out of scope.
| `Could not find package.json for package: ${ packageName }` | ||
| ); | ||
| } | ||
| const packageEntry = PACKAGES.get( packageName ); |
There was a problem hiding this comment.
transpilePackage now assumes PACKAGES.get( packageName ) always returns an entry. If packageName is ever missing/undefined (e.g., due to duplicate full package names or unexpected dependency graph output), this will throw a generic TypeError. Consider adding an explicit guard and throwing a more actionable error (similar to the prior “Could not find package.json…” check).
Suggested change
| const packageEntry = PACKAGES.get( packageName ); | |
| const packageEntry = PACKAGES.get( packageName ); | |
| if ( ! packageEntry ) { | |
| throw new Error( | |
| `Could not find package entry for "${ packageName }" in the package map.` | |
| ); | |
| } |
Contributor
Author
There was a problem hiding this comment.
All callers pass names from PACKAGES iterations, so get() always returns a defined entry. The removed check was also dead code — getPackageInfoFromFile throws on missing files, never returns null.
Comment on lines
+1768
to
+1770
| shortToFull.set( pkg, entry.packageJson.name ); | ||
| fullToShort.set( entry.packageJson.name, pkg ); | ||
| fullToPackageJson.set( entry.packageJson.name, entry.packageJson ); |
There was a problem hiding this comment.
When building fullToShort / fullToPackageJson, duplicate package.json.name values will silently overwrite earlier entries. With multi-root discovery this becomes more likely and can break build ordering and watch rebundles. Consider detecting duplicates (e.g., if fullToShort already has the full name) and throwing a clear error that includes both colliding package directories.
Suggested change
| shortToFull.set( pkg, entry.packageJson.name ); | |
| fullToShort.set( entry.packageJson.name, pkg ); | |
| fullToPackageJson.set( entry.packageJson.name, entry.packageJson ); | |
| const fullName = entry.packageJson.name; | |
| if ( fullToShort.has( fullName ) ) { | |
| const existingPackage = fullToShort.get( fullName ); | |
| throw new Error( | |
| `Duplicate package.json name "${ fullName }" detected for package directories "${ existingPackage }" and "${ pkg }". Package names must be unique across all discovered roots.` | |
| ); | |
| } | |
| shortToFull.set( pkg, fullName ); | |
| fullToShort.set( fullName, pkg ); | |
| fullToPackageJson.set( fullName, entry.packageJson ); |
Contributor
Author
There was a problem hiding this comment.
Valid edge case, but the existing code doesn't validate any similar collision. Keeping consistent — can address in a follow-up if needed.
Comment on lines
1579
to
1584
| return Array.from( PACKAGES.values() ).some( ( entry ) => { | ||
| const packagePath = normalizePath( | ||
| path.join( 'packages', packageName ) | ||
| path.relative( ROOT_DIR, entry.dir ) | ||
| ); | ||
| return relativePath.startsWith( packagePath + '/' ); | ||
| } ); |
There was a problem hiding this comment.
isPackageSourceFile converts the package registry to an array on every call (Array.from( PACKAGES.values() )). Since this runs for every watcher event, consider iterating for ( const entry of PACKAGES.values() ) directly to avoid repeated allocations.
Contributor
Author
There was a problem hiding this comment.
Good catch — applied. Now uses for...of PACKAGES.values() directly.
|
Flaky tests detected in 491abf1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24662505238
|
retrofox
force-pushed
the
add/wp-build-sources-config
branch
from
8221198 to
076e1fe
Compare
t-hamano
added
[Type] Enhancement
retrofox
changed the title
retrofox
force-pushed
the
add/wp-build-sources-config
branch
from
94c8f76 to
79fd828
Compare
retrofox
added a commit
to Automattic/jetpack
that referenced
this pull request
Apr 14, 2026
Patches @wordpress/build@0.11.0 to support packageSources config for cross-directory package discovery. Per-package externals instead of namespace-wide inference, skip transpilation for external sources. Adds wpScriptModuleExports to number-formatters. Upstream: WordPress/gutenberg#77226
Open
2 tasks
retrofox
added a commit
to Automattic/jetpack
that referenced
this pull request
Apr 14, 2026
Patches @wordpress/build@0.11.0 to support packageSources config for cross-directory package discovery. Per-package externals instead of namespace-wide inference, skip transpilation for external sources. Adds wpScriptModuleExports to number-formatters. Upstream: WordPress/gutenberg#77226
retrofox
added a commit
to Automattic/jetpack
that referenced
this pull request
Apr 15, 2026
Patches @wordpress/build@0.11.0 to support packageSources config for cross-directory package discovery. Per-package externals instead of namespace-wide inference, skip transpilation for external sources. Adds wpScriptModuleExports to number-formatters. Upstream: WordPress/gutenberg#77226
retrofox
added a commit
to Automattic/jetpack
that referenced
this pull request
Apr 16, 2026
Patches @wordpress/build@0.11.0 to support packageSources config for cross-directory package discovery. Per-package externals instead of namespace-wide inference, skip transpilation for external sources. Adds wpScriptModuleExports to number-formatters. Upstream: WordPress/gutenberg#77226
convert getAllPackages() from string[] to Map<name, {dir, packageJson}>
and update all call sites to use registry lookups
support directory paths and npm package names for cross-directory package discovery. Named sources preserve their npm identity as script-module IDs. Fix package.json resolution for strict exports.
replace scope-wide externalization (@scope/*) with exact package matching for named sources. Skip transpilation for external packages.
retrofox
force-pushed
the
add/wp-build-sources-config
branch
from
8a0698e to
dc47e96
Compare
retrofox
added a commit
to Automattic/jetpack
that referenced
this pull request
Apr 17, 2026
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.
Contributor
Author
|
While validating wp-build's templates currently The fix was correct for that scenario but applied universally. With Rather than bundling a template change into this PR, I opened #77465 which scopes the deregister to |
Contributor
|
I don't think separate plugins should register the same module names. It creates conflicts if the plugins require different versions of these modules. |
retrofox
changed the title
Contributor
Author
Rethought this after your feedback. I've reduced the scope: packageSources now only extends directory scanning. Each plugin still registers its own packages under its own namespace, same as local ones. No more preserved npm identity, no cross-plugin shared IDs. The script-module registration model is unchanged. If sharing IDs across plugins is worth pursuing later, that's a separate discussion (likely involving a host-plugin pattern or explicit version coordination), and I'd rather have it on its own merits. |
Contributor
|
I don't want to block anything here but why packageSource should be configurable and not "routesSource" or "widgets", or "blocks" or "fields" later... If we can avoid this config, personally I'd avoid it. |
Contributor
Author
|
Valid concern, and honestly, I'm aware that a seemingly small API addition can open a Pandora's box. Once there's one The distinction I'd draw, though, is that If we go this route, I think clear documentation framing it as "the only convention designed to be shared" would keep the door from swinging open for the rest. The alternative, if that framing still feels fragile, is to drop the explicit config entirely and instead follow the package manager's own workspace declaration ( No new API surface on our side. We'd respect a convention that already describes where packages live: With this,
No worries, I hear you. |
Contributor
|
the relying on workspaces is interesting but doesn't work because "routes" should be in the workspaces as well (and probably all the other stuff) for npm installs to work properly. |
Contributor
Author
Thanks for the advice. That brings us back to a few options: (a) the original explicit (b) No change here, and on the Jetpack side, find a way to build ES modules from packages that live outside the plugin context. Something pragmatic like symlinks as a quick, admittedly hacky workaround; (c) Go further and introduce a global registry at the Jetpack level that hosts shared modules centrally (analogous to how Core hosts |
Contributor
|
I would love to leave option (a) as the last option but I'm open to it. Just let me know how you want to proceed. |
Contributor
Author
|
I'm going to draft this PR and try to find a solution on the plugin/consumer side. Let's see. |
retrofox
added a commit
that referenced
this pull request
May 29, 2026
Decouples script-module identity from wpPlugin.packageNamespace by reading each package's own name field as the script-module ID and externalizing internal-package imports against an exact-name registry. The PHP registry, the asset manifest, and wp_register_script_module already treat IDs as opaque strings, so the npm name now survives end-to-end (npm name === import specifier === script-module ID). No-op for Core (every package's name already matches the legacy derivation). Unblocks consumers whose owned npm scope differs from packageNamespace from keeping a single identifier across pnpm, IDE, and the WordPress runtime. Replaces #77226 with a removal-of-coupling framing.
retrofox
added a commit
that referenced
this pull request
May 29, 2026
Decouples script-module identity from wpPlugin.packageNamespace by reading each package's own name field as the script-module ID and externalizing internal-package imports against an exact-name registry. The PHP registry, the asset manifest, and wp_register_script_module already treat IDs as opaque strings, so the npm name now survives end-to-end (npm name === import specifier === script-module ID). No-op for Core (every package's name already matches the legacy derivation). Unblocks consumers whose owned npm scope differs from packageNamespace from keeping a single identifier across pnpm, IDE, and the WordPress runtime. Replaces #77226 with a removal-of-coupling framing.
Contributor
Author
|
Closing this in favor of #78822, which takes the direction suggested here: instead of adding a That addresses the main concern in this thread (avoiding an The companion #77465 scopes the generated Thanks @youknowriad for the feedback that steered this away from a config-based approach. Continuing on #78822. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Adds a
wpPlugin.packageSourcesfield to@wordpress/buildthat accepts an array of directory paths. Each entry is scanned for packages in addition to the default./packages/directory.Closes #77225.
Why?
@wordpress/builddiscovers packages by scanning./packages/*relative tocwd. This path is hardcoded. Monorepos where plugins share internal packages that live outside./packages/cannot have those packages compiled by the tool — they get inlined into each consumer's bundle instead of being emitted as separate script modules.The announcement article explicitly invites raising this kind of constraint:
How?
1. Refactor PACKAGES to a registry Map
Converts
getAllPackages()from returningstring[]toMap<name, { dir, packageJson }>. All call sites look packages up through the registry instead of recomputingpath.join( PACKAGES_DIR, name ). This is required because packages no longer live in a single directory and it also avoids redundant disk reads (parsedpackage.jsonis cached in the entry).2. Add
wpPlugin.packageSourcesconfigThe
packageSourcesarray accepts directory paths resolved relative to the project root:{ "wpPlugin": { "packageSources": [ "../js-packages" ] } }With this configuration the tool scans
./packages/*(always) and../js-packages/*. Discovered packages are treated identically to local ones: they go through the same transpile and bundle pipeline and are registered under the plugin'spackageNamespace.Local packages always take priority. If a package discovered through
packageSourceshas the same directory name as a local one, the local entry wins (first-match-wins).Files changed
packages/wp-build/lib/build.mjspackageSourcesdirectory scanningpackages/wp-build/README.mdwpPlugin.packageSourcespackages/wp-build/CHANGELOG.mdScope
This PR is deliberately limited to build-tool discovery. Each plugin still registers its own packages under its own
packageNamespace— no changes to the script-module registration model, no cross-plugin shared IDs, no changes to the externals plugin beyond threading the new discovery through. The registry output is indistinguishable from what would be produced if the discovered packages lived in./packages/.Discussion about whether multiple plugins should be able to share the same script-module ID (and the trade-offs involved) is intentionally out of scope here.
Testing
Tested on a monorepo with the following layout:
Consumer config:
{ "wpPlugin": { "packageNamespace": "my-plugin", "packageSources": [ "../../js-packages" ] } }Without
packageSources: the shared package is inlined into every route bundle that imports it.With
packageSources: [ "../../js-packages" ]:build/modules/formatters/index.min.js(alongsidebuild/modules/init/index.min.js)registry.phpregisters@my-plugin/formattersand@my-plugin/initunder the consumer's namespace@my-plugin/formattersas a script-module dependency — no inline copyimport()) generate'import' => 'dynamic'for lazy loadingValidated:
packageNamespace, same as local ones'import' => 'static'dependencies'import' => 'dynamic'dependencies (lazy loading)packageSources, behavior is identical to beforeCompatibility
fast-glob,path, andfsnormalizePath()andpath.resolve()