doc: include global node_modules in require.resolve description #20534

musgravejw · 2018-05-05T05:45:05Z

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

vsemozhetbyt · 2018-05-05T10:10:35Z

doc/api/modules.md

-    these paths is used as a starting point for the module resolution algorithm,
-    meaning that the `node_modules` hierarchy is checked from this location.
+    paths are used instead of the default resolution paths, with the exception
+    of global folders like ~/.node_modules, which are always included. Note that


Nit: ~/.node_modules -> `~/.node_modules`?

(If so, this line will need rewrapping at 80 characters.)

BridgeAR · 2018-05-05T12:38:59Z

LGTM with the comments addressed.

richardlau · 2018-05-05T13:14:13Z

Looks like GitHub's web interface no longer fully works on IOS 10 so I can't properly review this to request changes.

Please could you rename GLOBAL_NODE_MODULES to something like GLOBAL_FOLDERS since none of the global folders is called node_modules and to try to minimise any confusion with npm's global installations (which are not the same thing).

cc @nodejs/modules

musgravejw · 2018-05-05T15:14:18Z

Updated.

vsemozhetbyt · 2018-05-05T15:58:36Z

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

vsemozhetbyt · 2018-05-05T15:59:16Z

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

ljharb · 2018-05-05T16:05:46Z

doc/api/modules.md

 2. for each DIR in DIRS:
   a. LOAD_AS_FILE(DIR/X)
   b. LOAD_AS_DIRECTORY(DIR/X)

 NODE_MODULES_PATHS(START)
 1. let PARTS = path split(START)
 2. let I = count of PARTS - 1
-3. let DIRS = []
+3. let DIRS = [ GLOBAL_FOLDERS ]


The term “GLOBAL_FOLDERS” doesn’t appear to be defined anywhere; if you mean the list in line 424, could you alter that list as well to use this identical term?

Ideally, this term would also link down to that section.

@ljharb What would be the best way to capture this? The main heading is regarding "loading from the global folders." Would it make sense to say Additionally, Node.js will search in the following global folders:?

Even just a sentence in the below section like “This list of GLOBAL_FOLDERS:” or something

It’d be cool if the mention in the algorithm itself could also be linked.

vsemozhetbyt · 2018-05-07T00:38:29Z

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

xtuc · 2018-05-07T07:58:30Z

doc/api/modules.md

 2. for each DIR in DIRS:
   a. LOAD_AS_FILE(DIR/X)
   b. LOAD_AS_DIRECTORY(DIR/X)

 NODE_MODULES_PATHS(START)
 1. let PARTS = path split(START)
 2. let I = count of PARTS - 1
-3. let DIRS = []
+3. let DIRS = [GLOBAL_FOLDERS](#modules_loading_from_the_global_folders)


Will this link render on the website? On github it's not.

It seems not up-to-date, or is it just me?

Sorry, I misunderstood the note.

xtuc · 2018-05-07T07:59:59Z

doc/api/modules.md

-    meaning that the `node_modules` hierarchy is checked from this location.
+    paths are used instead of the default resolution paths, with the exception
+    of [GLOBAL_FOLDERS](#modules_loading_from_the_global_folders) like
+    `~/.node_modules`, which are always included. Note that each of these paths


$HOME/.node_modules should be preferred, tild is from bash.

$HOME/.node_modules would also be consistent with the list in the Loading from the global folders section.

trivikr · 2018-05-08T03:02:33Z

doc/api/modules.md

-    these paths is used as a starting point for the module resolution algorithm,
-    meaning that the `node_modules` hierarchy is checked from this location.
+    paths are used instead of the default resolution paths, with the exception
+    of [GLOBAL_FOLDERS](#modules_loading_from_the_global_folders) like


Since the link #modules_loading_from_the_global_folders is used twice in the document, it can be moved to bottom of the page as follows:

vsemozhetbyt · 2018-05-08T19:05:58Z

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

richardlau · 2018-05-08T20:14:47Z

@xtuc's comment still stands, the change doesn't render as a link in GitHub: https://github.com/musgravejw/node/blob/4b6bf5cca73c211007d99344522fb91c6b9668bf/doc/api/modules.md#all-together

ljharb · 2018-05-08T20:16:17Z

Since it's in a preformatted block, I don't think that's possible.

Either way, as long as I can "find in page" for "GLOBAL_FOLDERS", it's good, and i think i'd personally rather have the website be more usable with a link than have the markdown be less messy by omitting the link.

vsemozhetbyt · 2018-05-08T20:28:29Z

Yes, sorry, I'missed that this is a code block.

trivikr · 2018-05-09T00:47:36Z

@vsemozhetbyt #20534 (review) is not addressed yet

vsemozhetbyt · 2018-05-09T14:16:19Z

@musgravejw

After creating the bottom reference you can use it in the text as [GLOBAL_FOLDERS][].
We still need to delete the link from the code block as it is not rendered as a link in the HTML page.

vsemozhetbyt · 2018-05-09T17:26:21Z

doc/api/modules.md

-    `~/.node_modules`, which are always included. Note that each of these paths
-    is used as a starting point for the module resolution algorithm, meaning
-    that the `node_modules` hierarchy is checked from this location.
+    of [GLOBAL_FOLDERS][] like `$HOME/.node_modules`, which are always 


Nit: trailing space, but this can be fixed on landing)

vsemozhetbyt · 2018-05-09T17:29:14Z

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

@richardlau @trivikr PTAL.

doc/api/modules.md

 2. for each DIR in DIRS:
   a. LOAD_AS_FILE(DIR/X)
   b. LOAD_AS_DIRECTORY(DIR/X)

 NODE_MODULES_PATHS(START)
 1. let PARTS = path split(START)
 2. let I = count of PARTS - 1
-3. let DIRS = []
+3. let DIRS = [GLOBAL_FOLDERS]


PR-URL: #20534 Fixes: #18926 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

vsemozhetbyt · 2018-05-11T22:24:13Z

Landed in 6fd8022 (with a tiny fix: insert bottom reference in ASCII sort order).

Thank you!

PR-URL: #20534 Fixes: #18926 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels May 5, 2018

vsemozhetbyt reviewed May 5, 2018

View reviewed changes

doc: include global node_modules in require.resolve description

7967eca

musgravejw force-pushed the require-resolve branch from a72eab6 to 7967eca Compare May 5, 2018 15:13

vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label May 5, 2018

ljharb reviewed May 5, 2018

View reviewed changes

musgravejw force-pushed the require-resolve branch from b295820 to 354be97 Compare May 5, 2018 16:53

doc: rerence GLOBAL_FOLDERS and link to folder list

263ad12

musgravejw force-pushed the require-resolve branch from 354be97 to 263ad12 Compare May 5, 2018 17:22

ljharb approved these changes May 7, 2018

View reviewed changes

xtuc approved these changes May 7, 2018

View reviewed changes

xtuc reviewed May 7, 2018

View reviewed changes

inidaname approved these changes May 7, 2018

View reviewed changes

benjamingr approved these changes May 7, 2018

View reviewed changes

vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 7, 2018

trivikr reviewed May 8, 2018

View reviewed changes

vsemozhetbyt removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 8, 2018

musgravejw force-pushed the require-resolve branch from 2390fa1 to 4b6bf5c Compare May 8, 2018 18:51

This comment has been minimized.

Sign in to view

musgravejw force-pushed the require-resolve branch from 4b6bf5c to 75924f6 Compare May 9, 2018 14:02

doc: replace '~' with '$HOME', refactor

c7e33fe

musgravejw force-pushed the require-resolve branch from 75924f6 to c7e33fe Compare May 9, 2018 15:40

jasnell approved these changes May 9, 2018

View reviewed changes

vsemozhetbyt reviewed May 9, 2018

View reviewed changes

trivikr reviewed May 10, 2018

View reviewed changes

trivikr approved these changes May 10, 2018

View reviewed changes

vsemozhetbyt closed this May 11, 2018

addaleax mentioned this pull request May 14, 2018

v10.2.0 proposal #20724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: include global node_modules in require.resolve description #20534

doc: include global node_modules in require.resolve description #20534

musgravejw commented May 5, 2018 •

edited

Loading

vsemozhetbyt May 5, 2018

vsemozhetbyt May 5, 2018

BridgeAR commented May 5, 2018

richardlau commented May 5, 2018

musgravejw commented May 5, 2018

vsemozhetbyt commented May 5, 2018

vsemozhetbyt commented May 5, 2018

ljharb May 5, 2018

musgravejw May 5, 2018 •

edited

Loading

ljharb May 5, 2018

ljharb May 5, 2018

vsemozhetbyt commented May 7, 2018

xtuc May 7, 2018

This comment was marked as off-topic.

xtuc May 7, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

vsemozhetbyt May 8, 2018

xtuc May 7, 2018

richardlau May 8, 2018

trivikr May 8, 2018

vsemozhetbyt commented May 8, 2018

This comment has been minimized.

richardlau commented May 8, 2018

ljharb commented May 8, 2018

vsemozhetbyt commented May 8, 2018

trivikr commented May 9, 2018

vsemozhetbyt commented May 9, 2018

vsemozhetbyt May 9, 2018

vsemozhetbyt commented May 9, 2018

This comment was marked as resolved.

This comment was marked as resolved.

vsemozhetbyt commented May 11, 2018

doc: include global node_modules in require.resolve description #20534

doc: include global node_modules in require.resolve description #20534

Conversation

musgravejw commented May 5, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 5, 2018

richardlau commented May 5, 2018

musgravejw commented May 5, 2018

vsemozhetbyt commented May 5, 2018

vsemozhetbyt commented May 5, 2018

Choose a reason for hiding this comment

musgravejw May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented May 7, 2018

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented May 8, 2018

This comment has been minimized.

richardlau commented May 8, 2018

ljharb commented May 8, 2018

vsemozhetbyt commented May 8, 2018

trivikr commented May 9, 2018

vsemozhetbyt commented May 9, 2018

Choose a reason for hiding this comment

vsemozhetbyt commented May 9, 2018

This comment was marked as resolved.

This comment was marked as resolved.

vsemozhetbyt commented May 11, 2018

musgravejw commented May 5, 2018 •

edited

Loading

musgravejw May 5, 2018 •

edited

Loading