doc: fix some errors in esm resolution algorithms #50898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
doc
guybedford
approved these changes
Nov 24, 2023
GeoffreyBooth
approved these changes
Nov 24, 2023
lpinca
approved these changes
Nov 24, 2023
We've been meaning to expose this logic in a helper function. Is that something you'd be interested in implementing? |
aduh95
approved these changes
Nov 24, 2023
aduh95
added
author ready
|
@GeoffreyBooth, I don't know if I'd have the time to maintain a pure reference implementation, but my implementation here might be a good starting point for someone. Aside from using paths instead of URLs, it follows the documented algorithms as closely as possible. |
|
Force pushed another typo I forgot about. Section 3.3 of the same function also has confusing wording (I had to check the node.js code to implement it properly). I need to consider how to reword it though. |
|
I just meant exposing ours, like |
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/50898 ✔ Done loading data for nodejs/node/pull/50898 ----------------------------------- PR info ------------------------------------ Title doc: fix some errors in esm resolution algorithms (#50898) Author Christopher Jeffrey (JJ) (@chjj) Branch chjj:fix-esm-docs -> nodejs:main Labels doc, esm, author ready Commits 1 - doc: fix some errors in esm resolution algorithms Committers 1 - Christopher Jeffrey PR-URL: https://github.com/nodejs/node/pull/50898 Reviewed-By: Guy Bedford Reviewed-By: Geoffrey Booth Reviewed-By: Luigi Pinca Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50898 Reviewed-By: Guy Bedford Reviewed-By: Geoffrey Booth Reviewed-By: Luigi Pinca Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - doc: fix some errors in esm resolution algorithms ℹ This PR was created on Fri, 24 Nov 2023 17:13:06 GMT ✔ Approvals: 4 ✔ - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/50898#pullrequestreview-1748266109 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/50898#pullrequestreview-1748269208 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/50898#pullrequestreview-1748269330 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/50898#pullrequestreview-1748269950 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6996996694 |
|
@GeoffreyBooth, isn't this already going to be more-or-less exposed once I suppose CJS modules could access it if it were part of |
aduh95
approved these changes
Nov 29, 2023
aduh95
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 59f57d2 |
targos
pushed a commit
that referenced
this pull request
Dec 4, 2023
PR-URL: #50898 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
richardlau
pushed a commit
that referenced
this pull request
Mar 25, 2024
PR-URL: #50898 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two errors I discovered when implementing the ESM resolution algorithm from scratch.
PACKAGE_EXPORTS_RESOLVE-subpathis already prefixed with./due to the calling functions ensuring this.PACKAGE_TARGET_RESOLVE- A simple typo here. Order should be reversed.