[Bug]: Resolve imports field

[unional]

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.:

{
  "imports": {
    "./a.js": { /* ... */ }
  }
}

Also, it doesn't work correctly with conditions (also a problem from resolve.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

N/A

[unional]

After further investigation, I do not see any mention of recursive resolution.
So I make sure resolve.imports returns undefined in that case.

@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.

[SimenB]

@SimenB Do you know if there is a spec of the resolution on the NodeJS side?

https://nodejs.org/api/packages.html#subpath-imports

Unlike the "exports" field, the "imports" field permits mapping to external packages.
The resolution rules for the imports field are otherwise analogous to the exports field.

[unional]

https://nodejs.org/api/packages.html#subpath-imports

Yes, I do check that, and the implementation and tests are based on that.
But it is not an exact spec (e.g. didn't mention about recursive resolution).

[SimenB]

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)

[unional]

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.

[unional]

Added a section about the algorithm in the readme

1.2.4 is released

[unional]

Currently, the options only support and handles conditions.
The other options in resolve.exports are used to support older mechanisms it seems.

Let me know if you need to use other options.

[SimenB]

I'd prefer errors to be thrown. Any particular reason why they cannot be?

[unional]

Because I don't know how it is consumed. Does the resolve() function only called when the caller already determine there is an imports field in the package.json and the specifier starts with #?

If throwing error is the right way I can do that.

In @repobuddy/jest, I implement the resolver like this: https://github.com/repobuddy/jest/blob/main/packages/jest/ts/resolver.ts
and in resolve.exports, I saw it throws in most cases but also returns undefined: https://github.com/lukeed/resolve.exports/blob/master/src/index.js#L110
(if (exports) { ... } /* implicit else return undefined */)

If throwing error is the right way, one question I have is how to throw those internal errors (e.g. Throw an Invalid Module Specifier error), as they are inside internal/errors of Node.js: https://github.com/nodejs/node/blob/2af43f65055de2817eb7907d3a3d3b3a3de127f5/lib/internal/modules/esm/resolve.js#L46

I think the errors I need to throw are:

1. Invalid Module Specifier
2. Package Import Not Defined (if caller already asserted specifier starts with #)

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 package.json and resolve the target, and throw Package Import Not Defined error if unable to resolve.

But the resolve() function is only resolving the imports field using the passed-in package.json object.
So it is likely not its responsibility to throw.

Should I throw a generic error instead?

Reference: https://nodejs.org/api/esm.html#resolver-algorithm-specification

[unional]

FYI I'm updating the algorithm here: nodejs/node#46068

[unional]

Found this: https://github.com/yarnpkg/berry/blob/9780dfc239512f4c0287e9e2931213b06505d0fa/packages/yarnpkg-pnp/sources/node/errors.js#L5

Maybe I can do that for the Invalid Module Specifier error

[unional]

I took at crack at it, and confirmed what I suspect.

I can throw Invalid Module Specifier error, but not the Package Import Not Defined error because the error expects the packagePath and the base (of there it was imported from):

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.

[SimenB]

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

[unional]

Sure, working on exactly that. :) Will be ready in about 10 mins

[unional]

cyberuni/resolve.imports#22

[SimenB]

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 😀

[unional]

I have first created this PR to make contributing easier: #13721

[unional]

Working on the actual PR now

[unional]

One thing to note is that the current jest implementation does not support array pattern.

I'm going to also fix that in this PR (as the TS errors out)

[github-actions]

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.