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.

Sign up for GitHub

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

Jump to bottom

doc: fix an error in resolution algorithm steps #29940

Closed
wants to merge 1 commit into from
Closed

doc: fix an error in resolution algorithm steps #29940

wants to merge 1 commit into from

Conversation

alexzherdev
Copy link
Contributor

As it is, if X begins with './' or '/' or '../' (step 3), it reads as if it were possible for the algorithm to do a node_modules lookup (step 4) when 3.a or 3.b couldn't find the module. But that doesn't seem to reflect the actual logic.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Oct 11, 2019
ljharb
ljharb reviewed Oct 12, 2019
View reviewed changes
doc/api/modules.md
@@ -159,6 +159,7 @@ require(X) from module at path Y
3. If X begins with './' or '/' or '../'
a. LOAD_AS_FILE(Y + X)
b. LOAD_AS_DIRECTORY(Y + X)
c. THROW "not found"
4. LOAD_NODE_MODULES(X, dirname(Y))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, could this be prefixed with “else”?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of that but noticed that other steps here prefer throwing in the if instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's consistent with the other blocks.

@ljharb
Copy link
Member

ljharb commented Oct 12, 2019

cc @nodejs/modules-active-members

bnoordhuis
bnoordhuis approved these changes Oct 14, 2019
View reviewed changes
doc/api/modules.md
@@ -159,6 +159,7 @@ require(X) from module at path Y
3. If X begins with './' or '/' or '../'
a. LOAD_AS_FILE(Y + X)
b. LOAD_AS_DIRECTORY(Y + X)
c. THROW "not found"
4. LOAD_NODE_MODULES(X, dirname(Y))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's consistent with the other blocks.

jkrems
jkrems approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct.

doc: fix an error in resolution algorithm steps
1a141ba 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.
@alexzherdev
Copy link
Contributor Author

Fixed the commit message to wrap at 72 characters.

@danbev
Copy link
Contributor

danbev commented Oct 16, 2019

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3998/

@alexzherdev
Copy link
Contributor Author

Waiting for #30117 to land before resolving the conflicts.

@jkrems
Copy link
Contributor

jkrems commented Oct 29, 2019 •
edited
Loading

Landed that PR, should be safe to rebase now!

@alexzherdev
Copy link
Contributor Author

Huh, looks like there's no conflicts here anymore 😄

@jkrems jkrems self-assigned this Oct 30, 2019
jkrems pushed a commit that referenced this pull request Oct 30, 2019
@jkrems
doc: fix an error in resolution algorithm steps
6f39f10 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.

PR-URL: #29940
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jkrems
Copy link
Contributor

jkrems commented Oct 30, 2019

Landed in 6f39f10

@jkrems jkrems closed this Oct 30, 2019
@jkrems jkrems removed their assignment Oct 30, 2019
@alexzherdev alexzherdev deleted the patch-1 branch October 30, 2019 15:33
targos pushed a commit that referenced this pull request Nov 5, 2019
@targos
doc: fix an error in resolution algorithm steps
dfb4a24 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.

PR-URL: #29940
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Nov 5, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@targos
doc: fix an error in resolution algorithm steps
a474541 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.

PR-URL: #29940
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@targos
doc: fix an error in resolution algorithm steps
d85bfab 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.

PR-URL: #29940
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@targos
doc: fix an error in resolution algorithm steps
4894902 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.

PR-URL: #29940
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@targos
doc: fix an error in resolution algorithm steps
d0f5bc1 
As it is, if `X begins with './' or '/' or '../'` (step 3), it reads
as if it were possible for the algorithm to do a node_modules lookup
(step 4). But that doesn't seem to reflect the actual logic.

PR-URL: #29940
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@antsmartian antsmartian antsmartian approved these changes

@jkrems jkrems jkrems approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@alexzherdev @ljharb @danbev @jkrems @bnoordhuis @jasnell @antsmartian @BridgeAR @nodejs-github-bot