Skip to content
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

Ambiguous steps in PACKAGE_IMPORTS_EXPORTS_RESOLVE reference documentation #53206

Closed
Mrtenz opened this issue May 29, 2024 · 8 comments · Fixed by #53215
Closed

Ambiguous steps in PACKAGE_IMPORTS_EXPORTS_RESOLVE reference documentation #53206

Mrtenz opened this issue May 29, 2024 · 8 comments · Fixed by #53215
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders

Comments

@Mrtenz
Copy link
Contributor

Mrtenz commented May 29, 2024

Affected URL(s)

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

Description of the problem

I was looking at the ESM resolution algorithm documentation, and stumbled upon this:

PACKAGE_IMPORTS_EXPORTS_RESOLVE:

  1. [...]
  2. Let expansionKeys be the list of keys of matchObj containing only a single "*", sorted by the sorting function PATTERN_KEY_COMPARE which orders in descending order of specificity.
  3. [...]

(2) could be interpreted in several ways, including:

  1. List of keys of matchObj, sorted, filtered by keys containing only a single "*".
  2. List of keys of matchObj, filtered by keys containing only a single "*", sorted. I think the end result is the same in this case, though.
  3. List of keys of matchObj, where the value contains only a single "*", sorted by keys.

Assuming (1) or (2) here is correct, the implementation of PATTERN_KEY_COMPARE seems odd:

[...]
3. Let baseLengthA be the index of "" in keyA plus one, if keyA contains "", or the length of keyA otherwise.
4. Let baseLengthB be the index of "" in keyB plus one, if keyB contains "", or the length of keyB otherwise.
[...]
7. If keyA does not contain "", return 1.
8. If keyB does not contain "
", return -1.
[...]

From my understanding, neither keyA nor keyB can not contain a "*" at this point since it's filtered out. I looked at implementations of the module resolution, like Webpack's enhanced-resolve, and it seems like previously these imports or exports could end with a / instead of containing a *:

https:/webpack/enhanced-resolve/blob/e38970852a7f89b694f1053e285195511764a900/lib/util/entrypoints.js#L314-L322

However, this does not seem to work in the current Node.js version (tested on v20.12.2 at least), and is not described in the reference of PACKAGE_IMPORTS_EXPORTS_RESOLVE. Are the docs outdated? Is there some ambiguity here? Or am I simply misunderstanding the docs?

@Mrtenz Mrtenz added the doc Issues and PRs related to the documentations. label May 29, 2024
@RedYetiDev RedYetiDev added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels May 29, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented May 30, 2024

@nodejs/loaders

(BTW 1 & 2 return the same result)

@Mrtenz
Copy link
Contributor Author

Mrtenz commented May 30, 2024

(BTW 1 & 2 return the same result)

I figured that as well after writing it. 😅 Still, there is some ambiguity here, since it could also be interpreted as (3).

@aduh95
Copy link
Contributor

aduh95 commented May 30, 2024

I personally don’t find it confusing (we’re talking about the key, that would make little sense to involve the value at this point), and even if you misinterpret it, it wouldn’t be too bad because the value would also typically contain a single *. As always, PRs welcome to clarify the docs.

@Mrtenz
Copy link
Contributor Author

Mrtenz commented May 30, 2024

Fair enough. But if it's the case that the key always contains a single "*", the reference of PATTERN_KEY_COMPARE seems to have unnecessary steps. Happy to open a PR to update this.

@aduh95
Copy link
Contributor

aduh95 commented May 30, 2024

Let me rephrase that: value associated with keys that contains a single * char will typically also contain a single * char. Keys do not always contain a single * char.

Example of a valid "exports" map:

{
  "exports": {
    "./someKey/noStar": "./someFile.mjs",
    "./somePatternKey/*": "./someDir/*.mjs"

  }
}

@Mrtenz
Copy link
Contributor Author

Mrtenz commented May 30, 2024

I understand that some keys don't have a "*", but the reference for PACKAGE_IMPORTS_EXPORTS_RESOLVE mentions:

Let expansionKeys be the list of keys of matchObj containing only a single "*"

So I assume an implementation for this in JS could be:

const expansionKeys = Object.keys(matchObj).filter(key => count(key, "*") === 1);

After which expansionKeys is sorted by PATTERN_KEY_COMPARE. Yet PATTERN_KEY_COMPARE includes cases for when a key does not contain "*", which is never the case, right? It's never called for keys that don't contain a "*".

Object.keys(matchObj)
  .filter(key => count(key, "*") === 1)
  .sort(PATTERN_KEY_COMPARE); // The keys here always contain a "*"

@aduh95
Copy link
Contributor

aduh95 commented May 30, 2024

Ah I see, sorry for the misunderstanding! I think that's a relic from a time where there was another another way to define subpath patterns which was removed in #40121, and I guess PATTERN_KEY_COMPARE was never updated, good catch. If you want to PR a fix, that would be much appreciated.

@Mrtenz
Copy link
Contributor Author

Mrtenz commented May 30, 2024

Opened a PR here: #53215.

nodejs-github-bot pushed a commit that referenced this issue Jun 5, 2024
PR-URL: #53215
Fixes: #53206
Refs: #40121
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jun 7, 2024
PR-URL: #53215
Fixes: #53206
Refs: #40121
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53215
Fixes: #53206
Refs: #40121
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53215
Fixes: #53206
Refs: #40121
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants