module: fix loading from global folders on Windows

[richardlau]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

According to the module documentation:

Additionally, Node.js will search in the following locations:

    1: $HOME/.node_modules
    2: $HOME/.node_libraries
    3: $PREFIX/lib/node

Where $HOME is the user's home directory, and $PREFIX is Node.js's configured node_prefix.

Loading from $PREFIX/lib/node is broken on Windows as the code is calculating $PREFIX/lib/node
relative to process.execPath, but on Windows process.execPath is $PREFIX\node.exe whereas everywhere else process.execPath is $PREFIX/bin/node (where $PREFIX is the root of the
installed Node.js).

e.g. If Node.js is in c:\node, the code is adding c:\lib\node to the lookup path instead of c:\node\lib\node:

C:\node>set NODE_DEBUG=module

C:\node>node nosuchfile
MODULE 12780: looking for "C:\\node\\nosuchfile" in ["C:\\Users\\IBM_ADMIN\\.nod
e_modules","C:\\Users\\IBM_ADMIN\\.node_libraries","C:\\lib\\node"]
module.js:472
    throw err;
    ^

Error: Cannot find module 'C:\node\nosuchfile'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:535:3

C:\node>

[gibfahn]

cc/ @nodejs/platform-windows

I feel like this would be a semver-patch change, as it's fixing the actual behaviour to match the docs.

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

[jasnell]

Given past history, we need to be very careful with changes to the module loader logic. @nodejs/ctc ... thoughts on this?
/cc @nodejs/platform-windows

[joaocgreis]

There was already some discussion of this in #6434 , but no decision.

What about removing those 3 search locations altogether? As the doc says, "these are mostly for historic reasons", so perhaps it's time to see them gone. NODE_PATH can be used to achieve the same functionality explicitly. Was there any discussion around this in the past?

If not, I would rather see these paths changed to places that actually make sense on Windows (like %APPDATA%). If that change is still too big, this PR at least makes node behave as described in the docs, so I'm +1 here.

[richardlau]

cc @nodejs/testing since this PR also adds a new testcase to cover the documented behaviour.

[sam-github]

@joaocgreis Removing them is an even larger change to a system we are trying to not break! I doubt anyone has an appetite for adding new default paths. I don't think adding $PREFIX/lib/node into the default module resolution path would fly as a new feature, but @richardlau isn't proposing that. Its an existing documented feature, we just want it to work. We have a use-case for it, which is why we noticed its broken on Windows.

[sam-github]

Existing Windows behaviour seems so wrong, its hard to imagine any user-code relying on it. I'm +1. And its great to see tests.

[sam-github]

If I understand this correctly, on non-Windows we have

$PREFIX/
   bin/node
   lib/

and on Windows we have

$PREFIX\
  node.exe
  lib/

So the previous code was adding a path like c:\Program Files\lib to the default node search path.. which is just flat-out wrong?

I think the code above could benefit from a comment describing what its doing. Particularly if my guess above is wrong!

[richardlau]

Yes, your understanding is correct. I have added some comments to the code to clarify.

[richardlau]

Rebased and added a comment to the changed code as suggested by @sam-github.

I've also edited the PR description to (hopefully) make it clearer what problem this is trying to address with an example showing the current (incorrect) behavior.

[MylesBorins]

Cherry-picked to both v4 and v6. I plan to do a maintenance release for v4 including this and a couple other bugs that have been caught

[refack]

🎉🎉🎉🎉🎉🎉🎉🎉🎉
Ping @tniessen RE: global node_modules