-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[Bug]: Resolve imports field #13707
Comments
After further investigation, I do not see any mention of recursive resolution. @SimenB Do you know if there is a spec of the resolution on the NodeJS side? I referenced the spec in WICG/import-maps but it is not the exact spec NodeJS uses. I have also updated the readme of the package, mentioning the additional fixes over the implementation in resolve.exports. Please let me know if you have any questions. |
https://nodejs.org/api/packages.html#subpath-imports
|
Yes, I do check that, and the implementation and tests are based on that. |
If the docs are insufficient I'd open up an issue in Node's repo (and see what Node does at runtime in that case) |
https://nodejs.org/api/esm.html#resolver-algorithm-specification Just found this. Let me verify everything are in order. |
Added a section about the algorithm in the readme 1.2.4 is released |
Currently, the Let me know if you need to use other options. |
I'd prefer errors to be thrown. Any particular reason why they cannot be? |
Because I don't know how it is consumed. Does the If throwing error is the right way I can do that. In If throwing error is the right way, one question I have is how to throw those internal errors (e.g. I think the errors I need to throw are:
For the first one, it is defined in https://github.com/nodejs/node/blob/2af43f65055de2817eb7907d3a3d3b3a3de127f5/lib/internal/errors.js#L1155 I'm not too familiar with the Node.js internal code so don't know what is the error code, convention, or if there is a way to access that code and throw the same error. For the second one, the algorithm says: PACKAGE_IMPORTS_RESOLVE()
4. if packageURL is not null
1. pjson = READ_PACKGE_JSON()
2. If pjson.imports is a non-null Object
1. resovled = PACKGE_IMPORT_EXPORTS_RESOLVE()
2. if resolve is not null or undefined, return resolved
5. throws `Package Import Not Defined`
PACKAGE_IMPORTS_EXPORTS_RESOLVE()
1. ...
1. ...
2. return the result of PACKAGE_TARGET_RESOLVE() So the algorithm is describing the logic that actually read the But the Should I throw a generic error instead? Reference: https://nodejs.org/api/esm.html#resolver-algorithm-specification |
FYI I'm updating the algorithm here: nodejs/node#46068 |
Maybe I can do that for the |
I took at crack at it, and confirmed what I suspect. I can throw E('ERR_PACKAGE_PATH_NOT_EXPORTED', (pkgPath, subpath, base = undefined) => {
if (subpath === '.')
return `No "exports" main defined in ${pkgPath}package.json${base ?
` imported from ${base}` : ''}`;
return `Package subpath '${subpath}' is not defined by "exports" in ${
pkgPath}package.json${base ? ` imported from ${base}` : ''}`;
}, Error); So at best I can throw a generic error in that case. |
I can pass those things in so they can be used to enhance the error message? And fall back to a more generic error if missing |
Sure, working on exactly that. :) Will be ready in about 10 mins |
Very cool 👍 Wanna send a PR migrating to your module? If you wanna expand on our tests as well (to cover the use cases you lay out in the OP) that'd be swell 😀 |
I have first created this PR to make contributing easier: #13721 |
Working on the actual PR now |
One thing to note is that the current I'm going to also fix that in this PR (as the TS errors out) |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Version
29.3.1
Steps to reproduce
The package @okikio/resolve.imports used in #13705 is based on resolve.exports.
It doesn't work correctly with file extensions. e.g.:
Also, it doesn't work correctly with
conditions
(also a problem fromresolve.exports
).See:
#9430 (comment)
lukeed/resolve.exports#22
Expected behavior
All imports field resolutions should work
see: https://github.com/cyberuni/resolve.imports/blob/main/packages/resolve.imports/ts/index.spec.ts
Actual behavior
there are cases not working correctly.
Additional context
Can use resolve.imports, after I add the case to handle circular case
UPDATE: the spec does not support recursion, so the circular case is not possible.
resolve.imports returns
undefined
in those cases.Environment
The text was updated successfully, but these errors were encountered: