-
-
Notifications
You must be signed in to change notification settings - Fork 610
fix(node-resolve): resolve bare targets of package "imports" using export maps; avoid fileURLToPath(null) #1908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(node-resolve): resolve bare targets of package "imports" using export maps; avoid fileURLToPath(null) #1908
Conversation
…port maps; avoid fileURLToPath(null)\n\n- When an imports mapping points to a bare specifier (e.g. "#foo/*": "@scope/pkg/*"), resolve the target with the full export-map-aware algorithm instead of classic only.\n- Guard against null return to prevent TypeError from fileURLToPath(null).\n- Add test: imports pattern -> bare package that uses exports.\n\nFixes #1907
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix correctly routes bare imports
targets through the export-map-aware resolver and prevents a TypeError
by guarding against null
. Error messaging can be clearer by explicitly handling undefined
and by presenting a friendly path for the importer. Consider adding a negative test to lock in the improved failure behavior. No correctness or compatibility issues spotted in the changed logic.
Additional notes (2)
-
Readability |
packages/node-resolve/src/resolveImportSpecifiers.js:155-162
Good addition to guard againstnull
. Two improvements would make this error path more actionable and explicit: -
Be explicit about
null
vsundefined
rather than relying on== null
. -
Include a friendlier, filesystem-path version of
importer
(when it’s a file URL) and clarify that the failure is via package.jsonimports
.
This improves debuggability without changing behavior.
- Maintainability |
packages/node-resolve/test/package-entry-points.js:352-368
Nice positive test. Given the bug fix also changes the failure mode (avoidsfileURLToPath(null)
), it would be valuable to add a negative test that asserts aResolveError
with a clear message when animports
target cannot be resolved. This helps prevent regressions on the newly introduced error path.
Summary of changes
- In
resolveWithExportMap
, targets of packageimports
that are bare specifiers are now resolved using the export-map-aware path by delegating toresolveImportSpecifiers
instead of the classic resolver. - Added a null guard to prevent
fileURLToPath(null)
, throwing aResolveError
with a clearer message when resolution fails. - Added new test fixtures for a package whose
imports
pattern maps to a bare package that relies onexports
subpath mappings. - Added a test verifying resolution of
#foo/*
->exports-directory/foo/*
via a bare package that exposes subpaths throughexports
.
See: fix for handling bare targets in imports
and improved failure behavior.
@CharlieHelps use the PR template. future PRs that do not use the template will be closed without explanation. |
Expand this to see my work.
|
@shellscape thanks for the reminder—updated the PR description to use the repository template.
I’ll use the template for future PRs. |
@CharlieHelps what are my remaining credits/tokens? |
@shellscape I can’t see org billing from a GitHub thread. To check your remaining credits, open the Charlie dashboard and go to Manage → Billing & Usage (select your GitHub org if prompted). That page shows current balance and recent usage. (docs.charlielabs.ai) For context, PR reviews are free; typical costs are: messages 5 credits, a commit 20, simple PR 100, complex PR 200. Plan pages list monthly included credits (e.g., 10,000 on Startup). (charlielabs.ai) |
Rollup Plugin Name:
@rollup/plugin-node-resolve
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
resolves #1907
Description
Fixes resolution when a package.json
imports
entry maps to a bare specifier (e.g."#foo/*": "@scope/pkg/*"
). Previously these targets were resolved via the classic resolver which ignores the target package'sexports
, causing failures for packages that rely on export maps. The fix routes such targets through the export‑map‑aware path so subpath exports are honored.Also guards against
null
from resolution to avoidfileURLToPath(null)
, surfacing a clearerResolveError
instead.Included tests cover resolving
#foo/*
to a package that exposes subpaths viaexports
directory mappings. The new fixture isimports-bare-pattern-exports
and the positive case passes with the change.