Implementation of --adjacent-node-modules to support module symlinking to global cache

[ghost]

Checklist

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

Affected core subsystem(s)

Modules

Description of change

Branched from v7.0.0 tag.

Using --adjacent-node-modules cmdline arg, or setting NODE_ADJACENT_NODE_MODULES, adds additional search paths for resolving prefix-less requires (i.e. require('somelib') ).

Details provided in this issue

It's expected this PR will be rejected for now, but there were some who wanted to take a peak first.

Test results

[----------] Global test environment tear-down
[==========] 26 tests from 2 test cases ran. (233 ms total)
[ PASSED ] 26 tests.
running 'python tools\test.py --mode=release addons doctool known_issues message parallel sequential -J'
=== release test-http-chunk-problem ===
Path: parallel/test-http-chunk-problem
Command: C:\src\node\Release\node.exe C:\src\node\test\parallel\test-http-chunk-problem.js
--- TIMEOUT ---
=== release test-process-kill-null ===
Path: parallel/test-process-kill-null
Command: C:\src\node\Release\node.exe C:\src\node\test\parallel\test-process-kill-null.js
--- TIMEOUT ---
[03:37|% 100|+ 1257|- 2]: Done
running jslint

C:\src\node>

[sam-github]

you might want to PR this standalone if this a VC output file resulting from a node build

[joshgav]

Can this already be accomplished with the NODE_PATH env var? See https://github.com/nodejs/node/blob/master/lib/module.js#L629-L634

[ghost]

Unfortunately no. The search paths are dependent on where the require() call is made, and change accordingly. NODE_PATH is static within an execution.

Think of it this way. When node was first made, rather than appending '/node_modules' progressively to create a list of search paths like this (copied from docs):

/home/ry/projects/node_modules/bar.js
/home/ry/node_modules/bar.js
/home/node_modules/bar.js
/node_modules/bar.js

They could have just as easily appended '.node_modules' instead leading to this:

/home/ry/projects.node_modules/bar.js
/home/ry.node_modules/bar.js
/home.node_modules/bar.js
/.node_modules/bar.js

And the fundamental way in which the folder hierarchy provides precedence to enable multiple versions of a given module to be used, would still work precisely and as elegantly as it does today, because in both cases they're still folders aligned to the dependency hierarchy.

The key difference is that using '/node_modules' means the dependencies are physically underneath the dependent module's folder, and if there were multiple symlinks to that module's folder from several projects, the dependency tree's of each of those projects would always end up using the same dependencies when require()ing the shared symlinked module.

This is a problem because the version specs of dependencies in package.json are most always specified as a ranges, like '^1.0.0', which means, for various reasons, when a logical dependency tree from package.json is 'rendered' into a physical form you can get different versions of dependencies for various reasons, like just that time has passed and newer releases have been made.

And this means that, for all practical purposes, it's impossible to have one single copy of a module on a machine, that is referenced (symlinked) in many places, and still guarantee in each case the physical dependency trees are deterministic.

However, when '.node_modules' is used rather than '/node_modules', the folder is adjacent to the dependent module and not underneath it, and this is what allows physical dependency trees to vary the versions used on a per-project basis, just as happens today, except now all modules everywhere can by symlinked to a single physical copy, most likely stored in a global location.

This PR is extremely simple. When the search path list is being created, when ever '/node_modules' is appended to create a new path, this patch then adds a second path with '.node_modules' appended! Thats it! This lets modules still resolve exactly as today, via '/node_modules', but also by '.node_modules'. It's the best of all worlds: Backwards compatible, allows for the same dependency trees as today, but now only a single physical copy of a module need be physically stored somewhere. This means in practice dozens of gigabytes of storage is not wasted, that moving and deleting of node based projects is lickity split, and 'rendering' a dependency trees can by done entirely by symlinking to the modules, and not copying.. super super fast.

Reading this issue I created might offer more clarity, as it has an example of this.

[ghost]

@joshgav Just wanted to thank you for taking a seemingly objective first look at this, and attempting to offer another solution. It seems most others just hear "Modules" and "change" and immediately dismiss even trying to understand what the issue is, the simplicity of the solution, and the potential positive impact. Given your status here, was wondering if you had any insight on some of the key decision makers, etc., and how they might be better approached to at least take a fair look at this like you seemed to.

[jasnell]

I haven't yet had an opportunity to review the PR, but right off the bat this would need to target master and not the v7.x branch. We don't land new features or capabilities directly into the release branches unless they've been landed in master first.

[ghost]

@jasnell Thanks for the input. It was made quite clear by others this PR will never be accepted by node.js, simply because it's an enhancement to the Modules resolution algorithm (regardless that it's opt-in). The thinking was that developers who still want this feature, could build the branch knowing it was based on a released version of node, rather than an in-progress version. I was told by someone they would not review without a PR here, which is why I made the PR.

Would you still recommend it be based from master?

[jasnell]

Ok, given that then I think it's fine.

On Sunday, November 6, 2016, Paul D. Hester [email protected]
wrote:

@jasnell https://github.com/jasnell Thanks for the input. It was made
quite clear by others this PR will never be accepted by node.js, simply
because it's an enhancement to the Modules resolution algorithm (regardless
that it's opt-in). The thinking was that developers who still want this
feature, could build the branch knowing it was based on a released version
of node, rather than an in-progress version. I was told by someone they
would not review without a PR here, which is why I made the PR.

Would you still recommend it be based from master?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9456 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2efYWWKj30xGXNNOx8TEqDlue_AKKks5q7nRggaJpZM4KpKiO
.

[ghost]

Outside the politics, your opinion on the PR and what it enables would be greatly appreciated.