wp-build: Return null from getPackageInfo on resolve miss instead of throwing#78715
Open
chihsuan wants to merge 7 commits into
Open
wp-build: Return null from getPackageInfo on resolve miss instead of throwing#78715chihsuan wants to merge 7 commits into
chihsuan 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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @suzunn. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
chihsuan
added
[Type] Bug
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates @wordpress/build’s package resolution helper to honor its documented null return contract when a package can’t be resolved, preventing esbuild builds from failing when the externals plugin encounters namespaced imports that are resolved via tsconfig paths rather than installed dependencies.
Changes:
- Update
getPackageInfo()to catchrequire.resolve()failures and returnnull, caching negative lookups. - Add unit tests to verify missing packages return
null(and are cached), while installed packages still resolve. - Document the behavior change in
packages/wp-build’s changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/wp-build/lib/package-utils.mjs | Catch resolution failures in getPackageInfo() and cache null misses to avoid repeated throws. |
| packages/wp-build/lib/test/package-utils.test.js | Add unit coverage for “missing package returns null”, cache behavior, and installed-package resolution. |
| packages/wp-build/CHANGELOG.md | Add an Unreleased bugfix entry describing the crash prevention and new null behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Size Change: 0 B Total Size: 8.21 MB ℹ️ View Unchanged
|
|
I like that this keeps the fix in One regression check that would be worth preserving somewhere, even if it is not wired as a package-local unit test yet: call The |
…ageinfo-null # Conflicts: # packages/wp-build/CHANGELOG.md
This was referenced May 29, 2026
…ageinfo-null # Conflicts: # packages/wp-build/CHANGELOG.md
lib/test is excluded from the tsconfig type-check.
Contributor
|
@chihsuan, I took the liberty of pushing a commit adding unit tests. The "no automated test" point can be handled without the separate
But adding With that, a small unit test at
|
retrofox
approved these changes
Jun 3, 2026
Contributor
There was a problem hiding this comment.
Having looked at this closely, we find the approach coherent.
It restores getPackageInfo()'s documented PackageJson | null contract, which the externals plugin already relies on: its if ( ! packageJson ) return undefined guard was effectively dead code while the function threw.
Falling through to esbuild's own resolution then bundles the import inline, the same treatment a found-but-non-wpScript package already gets, so no new resolution path is introduced.
And narrowing the swallow to MODULE_NOT_FOUND / ERR_PACKAGE_PATH_NOT_EXPORTED while rethrowing the rest keeps genuine resolver errors visible.
Stepping back, @wordpress/build feels like a tool that, if we agree on it, could be shared and reused not only within Core but also by external plugins, even if it isn't fully ready for that yet.
If we're aligned on that direction, then not just this PR but further adjustments start to become necessary along the way. So if we agree with that development, merging this one makes sense.
cc @youknowriad
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?
Makes
@wordpress/build'sgetPackageInfo()returnnullwhen a package can't be resolved, instead of throwing. This stops the build from crashing on a namespaced import that isn't an installed dependency (e.g. one resolved viatsconfigpaths).Fixes #78714.
Why?
getPackageInfo()'s JSDoc promises@return {PackageJson|null}, but it callsrequire.resolve()with notry/catch, so a miss throws rather than returningnull. Thewordpress-externalsplugin relies on the documented contract:gutenberg/packages/wp-build/lib/wordpress-externals-plugin.mjs
Lines 220 to 227 in 5c33957
Because the function throws first, that guard is dead code — the exception propagates out of the
onResolvecallback and esbuild fails the build:✘ [plugin: wordpress-externals] Cannot find module '@my-plugin/init/package.json'The other caller (
inferStyleDependenciesinbuild.mjs) already wraps the call intry/catch, which is why only the externals plugin crashes.How?
Wrap
require.resolve()intry/catchand returnnullfor a genuine resolution miss, caching the negative result so repeated misses don't re-trigger a throwing resolve. This activates the existingnullguards in both callers; no behaviour change for installed packages. The fallen-through import is then bundled inline by esbuild — the same treatment already given to a found-but-non-wpScriptpackage.Only the expected resolution-miss codes are treated as a miss:
MODULE_NOT_FOUND(not installed) andERR_PACKAGE_PATH_NOT_EXPORTED(installed butpackage.jsonisn't exposed) — in both cases the package's metadata can't be read, so falling through is correct. Any other error (invalid specifiers, unexpected resolver failures) is rethrown so real problems aren't silently swallowed.Testing Instructions
No automated test is added:
@wordpress/build'stsconfig.jsontype-checkslib/and has no test-runner globals in scope, so alib/test/*file would need separate tsconfig wiring. If we prefer to add a test in this PR, I can set this up and add a test.Verify manually instead:
@wordpress/build, add atsconfig.jsonwith apathsentry mapping a namespaced specifier to a local source dir, e.g."@my-plugin/init": [ "./src/init" ], where@my-plugin/initis not installed innode_modules.import { init } from '@my-plugin/init';.✘ [plugin: wordpress-externals] Cannot find module '@my-plugin/init/package.json'.tsconfigpathsdiscovery and bundles the module inline.