-
Notifications
You must be signed in to change notification settings - Fork 30k
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
module: Refine and unify exports resolution error handling #31625
Conversation
db65846
to
888fae1
Compare
//cc @nodejs/modules-active-members |
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
Thanks @addaleax for the review here, would appreciate your advice re how to properly do the check handling. |
@addaleax I've added all the checks you mentioned here now. Better docs / info around how exactly to these sorts of cases properly would definitely be useful for future reference. |
7f9c775
to
d2f5a07
Compare
d2f5a07
to
f6d4dac
Compare
PR-URL: #31625 Reviewed-By: Jan Krems <jan.krems@gmail.com>
Landed in 58de9b4. |
This comment has been minimized.
This comment has been minimized.
The CITGM failures were tracked down to causes unrelated to this PR in nodejs/citgm#785 (comment). |
PR-URL: #31625 Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford if this is something that should be in |
@codebytere thanks for letting me know about this - backport PR at #32287. |
This error happens by this change in [Node,js v13.10](nodejs/node#31625) and I think we can regard this problem as the regression of this library. For the future, we should wait nodejs/node#32107 but it is still in progress. This patch will try to workaround for it. ## Reference * nodejs/node#31625 * nodejs/node#32107 * babel/babel#11216
This error happens by this change in [Node,js v13.10](nodejs/node#31625) and I think we can regard this problem as the regression of this library. For the future, we should wait nodejs/node#32107 but it is still in progress. This patch will try to workaround for it. * nodejs/node#31625 * nodejs/node#32107 * babel/babel#11216
PR-URL: nodejs#31625 Reviewed-By: Jan Krems <jan.krems@gmail.com>
PR-URL: nodejs#31625 Reviewed-By: Jan Krems <jan.krems@gmail.com>
This PR includes a number of refinements to the error handling of exports resolutions in the ES module resolver:
imported from ...
part since they don't have natural error stacks likerequire()
does.invalid:url
or backtrack below the package boundary like../../../outside
."exports": { "./sub/": "./sub/" }
where the user attempts to require egpkg/sub/../../../asdf
orpkg/sub/node_modules/...
etc. The error thus distinguishes that the fault is with the requestor, not the package.json itself.The main semantic changes in this PR are then the following:
"exports": { "node": { "node": "not:valid" }, "default": "./valid.js" }
would before this PR always return thevalid.js
path, but after this PR always throw an ERR_INVALID_PACKAGE_TARGET error for thenot:valid
path. On the other hand,"exports": { "node": { "never": "./never.js" }, "default": "./valid.js" }
would fall through the "never" path fine. Array fallbacks continue to fall back on the validation errors though as designed."."
entry, or because there is no conditional resolution for the"."
entry (eg"exports": { "never": "./never.js" }
), the ERR_PACKAGE_PATH_NOT_EXPORTED error will immediately throw without doing any"main"
or"index.js"
lookups. This makes "exports" exhaustive in its definition of the package resolution, and simplifies the error handling consistency for exports in turn.Since its original proposal,
"exports"
has very much turned into a full replacement for"main"
. Having self-resolve entirely reliant on this field being set is also in line with this. I understand (2) may be the most controversial part of this PR with the biggest user-facing consequences, so I expect we should discuss this further in the next meeting. This PR can be adjusted further based on any resolutions. The logic connects around the error handling hence the monolith here.In addition, this PR fixes #31510 and the incorrect use of XOR instead of bit shift described in #31008 (review).
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes