Experimental: Symlinks Just Work

[ghost]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  The same tests pass/fail with and without the change
• tests and/or benchmarks are included
  At the end of the comment
• documentation is changed or added
  This is an intentionally undocumented, experimental opt-in switch
• commit message follows commit guidelines

Affected core subsystem(s)

Modules

Symlinks that Just Work

The request to merge this PR also comes with a request that for the time being, the opt-in command line switch that activates the change in behavior be left undocumented and considered experimental.

To fully realize what the switch enables will require changes to package managers, and will then need to be used and vetted by some representative portion of the ecosystem to uncover and address any potential issues, all of which will take some time.

This is ultimately to determine if the solution should be announced and broadly publicized because it works great, or removed from node because it fundamentally does not solve the problem in a way that can be reliably used on a daily basis, or it in any way breaks the current ecosystem.

What I've Experienced

I'm a professional software engineer of 35+ years who in some way uses node on a daily basis, and has been doing so for about 2 years.

I have 150+ projects on just one of my development machines, spanning front-end, back-end, and library type projects, from developing my own modules, modules as part of teams, and open source modules I've git cloned and have contributed to or used for learning.

I enjoy developing with node, except whenever I need to install or update or change module dependencies for a project, as the time it takes to always copy modules, or sometimes delete the node_modules directory and reinstall everything, is quite frustrating. It feels like torture by a thousand paper cuts.

I know that in many cases I've already used many of the module@versions being copied during an install, and they're already on my machine. I know symlinking exists, and from past experiences outside node know that if it could be used correctly, installs and node_modules directory deletion would go from taking a couple minutes to a couple of seconds. It's like pouring rubbing alcohol all over the paper cuts.

I can also see by looking at the average size of the node_modules directory in my projects, and the size of npm's module cache, even factoring in a 90% compression ratio on the tarballs, that all those copies are taking several orders more space than would be needed if node and npm could exploit symlinking to its fullest; I could get many Gigabytes of storage back.

I understand several attempts have been made to use symlinks, but they have often just not worked in ways that can be generally relied on, and have caused show-stopping issues with memory consumption, module dependency-version resolution, addon loading, filesystem cycles, and tooling failure. Because of this, practically speaking, symlinking just shouldn't be used during development, and the general perception is that it will never quite work.

What I'd Like

I'd like node and npm (or any package manager) to exploit symlinks to optimize in all possible ways how modules are stored on my machine. In other words, for a given module@version there should be only one physical copy on my machine, and it should be symlinked to from anywhere else it may happen to be used on my machine.

Other than telling node and npm its ok to maximally exploit symlinking, I'd like to never have to do something different in how I'm using development tooling, or how I author a module and its module dependencies, just because modules may have been symlinked vs having been copied. In other words, module behavior and tooling behavior should always work the same, like they have no idea nor care if the module's directory was a symlink or an actual copy.

I'd like all the current show-stopping issues with using symlinks to just disappear.

I'd like it to be easy to point a symlink away from the central copy to somewhere else in my development root folder, to support those situations where I'm concurrently developing dependent modules at the same time.

I'd require a means to alter a particular symlink within a given project, by replacing it with a copy, so in those rare cases I want to temporarily muck with dependencies source in order to understand some behavior or find a bug, I can do so without affecting the centrally stored copy.

I'd require this all to be done without breaking anything in any way with how node and npm work today.

I'd require that if there were a solution that I began to use, and then some issue arose, I could always very quickly and easily just go back to not using symlinks for the particular project, simply by deleting node_modules and telling npm to copy-install everything.

While it would be nice if this would work in other contexts, such as on production servers, I only require that it work this way during development on a development machine.

How it can be done

This PR enables node, when explicitly opted-into, and working in conjunction with sufficiently enhanced package managers conforming to a normalized use of symlinks, to leverage symlinks in a way that Just Works. Additionally, when deploying modules, package managers can easily ring every last drop of value out of symlinks to fully optimize the use and consumption of a given machine's filesystem, regardless of how many places a given module may be redundantly used across the entire machine.

It enables this while also addressing all know issues within node when using symlinks to module directories, and would not require developers alter the way in which they currently specify, author, and consume modules, or expect modules to behave. In other words, the only things that would need to be aware of symlinks would be node and package managers (and bundlers like webpack and their brethren); developers and tooling would not.

Because the behavior is opt-in, merging the PR will not change node's default behavior. When opted-into, except for one edge case, the behavior should be fully backwards compatible with respect to node's behavior during link-time dependency-version resolution.

The behavior-effecting changes in this PR (ie those absent housekeeping, like reading the command line switch) are about 10 lines within the locked Modules subsystem.

I fully appreciate how preserving node's node_modules based elegant mechanic is fundamental to the entire ecosystem, even when not directly being used by node, but by bundlers such as webpack. It is currently the de-facto mechanism for dealing with link-time dependency-version resolution for the javascript ecosystem, and ES6 Modules wont change that as they don't address the problem of versioning.

I believe this PR not only preserves that, but enhances it just as elegantly while allowing us to get symlinks that just work.

Adjacent Node Modules

In order to precisely control what dependencies get resolved for a given module, they must be stored in a subdirectory, node_modules, of the dependent module's directory. node's resolution logic does allow for shared modules to be stored in a common ancestor node_modules directory, but this is merely an optimization. If a shared module has more than one of its versions used anywhere in a given dependency tree, the package manager can only bubble one of the versions to some common ancestor, with the remaining most likely needing to be copied into each dependent module's node_modules subdirectory.

The node_modules subdirectory constraint, while extremely elegant in fundamentally dealing with link-time dependency-version resolution in those cases where several versions of a common module are used in a single tree, is the core reason modules cannot be centrally stored on a machine and then symlinked to from anywhere else used on the machine. It is also the core reason filesystem cycles can appear when using symlinks. (Note: The ied package manager is a little more clever in how it ensures modules resolve their dependencies, but it is also prevented from using centrally stored copies for the same fundamental reason)

Understanding why it's the cause of cycles should be straight forward, but some don't at first understand why it prevents centrally storing. It basically comes down to the fact that module dependencies specified in a package.json usually come with a version specifier that is a range of valid versions, and based on simply when a module is installed and what version of it's dependencies happen to be most recently released, the actual dependency versions used can change from install to install; i.e. from project to project. This is one reason npm implements a "shrinkwrap" option, and why yarn has a "lock file". If a module was centrally stored with it's dependents underneath it in its node_modules subdirectory, not matter if they be copies or symlinks, it would not be possible to honor the shrinkwrap or lock files between projects, or for the same project between developer machines.

The solution is to offer an additional, equivalently scoping directory as the node_modules directory, that is located adjacent to the dependent module's directory rather than underneath it. This solves both the cycle problem when using symlinks, rendering them physically impossible, and the problems with symlinking to a central copy, all in one fell swoop.

With this approach, the central copy of a module would never have a node_modules subdirectory. Instead, an adjacent equivalently scoping directory would be an actual directory within a given project, itself containing symlinks to the particular modules dependencies@version, thereby keeping them specific to their inclusion in that specific project, which then enables shrinkwrap and lock files to work as expected, and directory cycles to never occur.

Implementation

Implementing Adjacent Node Modules is incredibly simple to do and understand. When node processes a require() call whose request path is neither relative nor absolute (i.e. doesn't start with '..', '.', or '/'), the first thing it does is create an ordered list of directory paths to search, starting from the directory location of the .js that made the require() call, ensuring or suffixing '/node_modes' as necessary. A list of directories could look something like this:

src/projectA/node_modules/mod-A/node_modules/mod-B/node_modules
src/projectA/node_modules/mod-A/node_modules
src/projectA/node_modules

Making an ordered list of equivalently scoping adjacent node_modules directories, is as simple as changing '/node_modules' to '.node_modules' (the / becomes a .), so the list would now look like this:

src/projectA.node_modules/mod-A.node_modules/mod-B.node_modules
src/projectA.node_modules/mod-A.node_modules
src/projectA.node_modules

node would still mechanically implement link-time dependency-version resolution, practically speaking, in exactly the same way it currently does. But of course if this PR only did that, activating the new behavior would obviously break running node against trees deployed using '/node_modules'. The solution for that is also very simple; we interleave both paths in the search, giving priority to '/node_modules', and so now the search list looks something like this for deployments using '/node_modules' based tree structure:

src/projectA/node_modules/mod-A/node_modules/mod-B/node_modules
src/projectA/node_modules/mod-A/node_modules/mod-B.node_modules
src/projectA/node_modules/mod-A/node_modules
src/projectA/node_modules/mod-A.node_modules
src/projectA/node_modules
src/projectA.node_modules

and then like this for new deployments using the '.node_modules' based tree structure:

src/projectA.node_modules/mod-A.node_modules/mod-B/node_modules
src/projectA.node_modules/mod-A.node_modules/mod-B.node_modules
src/projectA.node_modules/mod-A/node_modules
src/projectA.node_modules/mod-A.node_modules
src/projectA/node_modules
src/projectA.node_modules

The actual implementation merely creates an additional search path by suffixing '.node_modules', right after node decides it needs to make a search path suffixing '/node_modules', where the same parent folder is used in each case; super simple. This means with the new behavior active, its not only backwards compatible, but both structures are interoperable, giving precedence to '/node_modules'.

I earlier alluded to one edge case that could be a problem, and that would be if someone named their module something like 'myModule.node_modules'; i.e. '.node_modules' was actually part of the modules full name. While not impossible, I would opinion a highly improbable thing to occur. Package managers could easily check for such a thing when installing, and prevent with a warning, and the developer could then decide to simply not use the new behavior.

Simultaneously Preserving and Following Symlinks

By default, node will always 'follow' symlinks, meaning it will get the 'real' path of a symlink, and use that to identify the module. In other words the __dirname of a module is always the 'real' path of where the module physically exists. The 'real' path is also the path used inside node to cache/map a path to a module instance. This ensures node never creates multiple instances of the same logical module, which prevents memory from being consumed unnecessarily, and prevents node from possibly crashing when trying to load the same addon twice. However, its at the expense that other kinds of resolutions relative to the module's __dirname now occur outside the symlink, which is often not what is wanted and prevents many uses of symlinks.

When node is told to --preserve-symlinks, it uses the symlink path for both the __dirname and its internal cache/mapping, and while it does let symlinks work a bit more like one would expect (kinda, as the directory of the entry.js passed on the command line is not preserved), the above problems can then occur regarding memory consumption and crashing from multiply loading the same addon.

The fix for this is also incredibly simple, but first lets understand how a module's path is, and is not, significant.

When following symlinks, if there are multiple symlinks to the same module, no matter what, the module will always have the same __dirname; it's 'real' path. In fact, from execution to execution, one could change the physical location, i.e. the 'real' path, update all the symlinks, and the program would still operate exactly like it did on the previous executions, even though at no time where any of the symlink paths used as the __dirname.

This makes it quite obvious the actual __dirname of a module isn't important to the modules behavior, but only to resolutions that are relative to it's __dirname. And this is what lets us fundamentally simultaneously preserve and follow symlinks. This is accomplished by no longer coupling the path we use to cache/map a module or addon instance, to the path we use to initialize its __dirname.

Implementation

With the new behavior active, the path we use to cache/map will always be the 'real' path of a module, but the path we use as the modules '__dirname' will always be the first symlink path that was used to initially load the module (or the real path if it wasn't a symlink to begin with). This means that the next symlink to that module that gets required(), will still get the first instance that was created, but that instance would still have its __dirname set to the symlink that it was first loadeded through. But going back to our previous thought experiment regarding changing the real path from run to run, it won't matter. However, because the __dirname is in fact still a symlink, things will work exactly as one would expect.

Also, unlike the --preserve-symlinks switch, this behavior is applied to the directory of the entry.js file passed to node on its command line, so that tooling and scripts that launch node with entry.js files somewhere down in the node_modules tree also still behave as expected when they're coming from a symlinked directory.

This does place and additional responsibility on package managers to ensure that in each case the symlinked modules will resolve their dependencies exactly the same, but that's an easy thing, and not part of node's behavior.

All Together

By implementing Adjacent Node Modules and Simultaneous Preservation and Resolution of symlinks, node can very easily and efficiently use symlinks all the time in all cases, without tooling and developers having to do anything different than they otherwise would.

However, there are going to be some issues with package managers and tooling. For example, package managers run preinstall, install, and postinstall lifecycle scripts. How and when those run would need to change. Also, some scripts simply launch node to run some module that was actually installed somewhere in the tree, and package managers will need to know to set the environment variable that activates the new behavior before spawning a process to run the node script.

There are also others things that might be issues, but I'm already working on a first POC fork/branch of yarn, and this is also why, for now, it would be nice to merge this PR, but keep it as a hidden and undocumented switch.

Lastly

I'm looking foward to answering any questions in more detail, and responding to any concerns. However, if you decide to comment or question, please, please, please don't say something like 'This will break things with ES6 Modules', or 'This will still likely break vast amounts of the ecosystem'. I can't respond to those types of comments, they're not at all helpful in bringing about technical awareness, and I will simply ignore them. Please reply with actionable, technically descriptive responses.

Test Results

[----------] Global test environment tear-down
[==========] 26 tests from 2 test cases ran. (211 ms total)
[  PASSED  ] 26 tests.
running 'python tools\test.py --mode=release  sequential parallel message gc inspector internet pummel'
=== 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 ---
=== release test-net-timeout ===
Path: gc/test-net-timeout
We should do 500 requests
Done: 22/500
Collected: 11/33
Done: 44/500
Collected: 33/55
Done: 66/500
Collected: 66/88
Done: 88/500
Collected: 77/99
events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: connect EADDRNOTAVAIL :::26086
    at Object.exports._errnoException (util.js:1022:11)
    at exports._exceptionWithHostPort (util.js:1045:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)
Command: C:\src\node\Release\node.exe --expose-gc C:\src\node\test\gc\test-net-timeout.js
=== release test-crypto-timing-safe-equal-benchmarks ===
Path: pummel/test-crypto-timing-safe-equal-benchmarks
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-crypto-timing-safe-equal-benchmarks.js
--- TIMEOUT ---
=== release test-dh-regr ===
Path: pummel/test-dh-regr
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-dh-regr.js
--- TIMEOUT ---
=== release test-fs-watch-file ===
Path: pummel/test-fs-watch-file
assert.js:355
    throw actual;
    ^

Error: "watchFile()" requires a listener function
    at Object.fs.watchFile (fs.js:1409:11)
    at C:\src\node\test\pummel\test-fs-watch-file.js:41:10
    at _tryBlock (assert.js:314:5)
    at _throws (assert.js:333:12)
    at Function.throws (assert.js:363:3)
    at Object.<anonymous> (C:\src\node\test\pummel\test-fs-watch-file.js:39:8)
    at Module._compile (module.js:602:32)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:519:32)
    at tryModuleLoad (module.js:478:12)
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-fs-watch-file.js
=== release test-http-client-reconnect-bug ===
Path: pummel/test-http-client-reconnect-bug
C:\src\node\test\pummel\test-http-client-reconnect-bug.js:11
  var client = http.createClient(common.PORT);
                    ^

TypeError: http.createClient is not a function
    at Server.<anonymous> (C:\src\node\test\pummel\test-http-client-reconnect-bug.js:11:21)
    at Server.<anonymous> (C:\src\node\test\common.js:422:15)
    at emitNone (events.js:86:13)
    at Server.emit (events.js:185:7)
    at emitListeningNT (net.js:1288:10)
    at _combinedTickCallback (internal/process/next_tick.js:71:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
    at Module.runMain (module.js:638:11)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-http-client-reconnect-bug.js
=== release test-https-ci-reneg-attack ===
Path: pummel/test-https-ci-reneg-attack
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-https-ci-reneg-attack.js
--- TIMEOUT ---
=== release test-net-connect-memleak ===
Path: pummel/test-net-connect-memleak
C:\src\node\test\pummel\test-net-connect-memleak.js:20
  for (let i = 0; i < 26; ++i) junk = junk.concat(junk);
                                           ^

RangeError: Invalid array length
    at Array.concat (<anonymous>)
    at Object.<anonymous> (C:\src\node\test\pummel\test-net-connect-memleak.js:20:44)
    at Module._compile (module.js:602:32)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:519:32)
    at tryModuleLoad (module.js:478:12)
    at Function.Module._load (module.js:470:3)
    at Module.runMain (module.js:636:10)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
Command: C:\src\node\Release\node.exe --expose-gc C:\src\node\test\pummel\test-net-connect-memleak.js
=== release test-tls-ci-reneg-attack ===
Path: pummel/test-tls-ci-reneg-attack
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-tls-ci-reneg-attack.js
--- TIMEOUT ---
=== release test-tls-connect-memleak ===
Path: pummel/test-tls-connect-memleak
C:\src\node\test\pummel\test-tls-connect-memleak.js:30
  for (let i = 0; i < 26; ++i) junk = junk.concat(junk);
                                           ^

RangeError: Invalid array length
    at Array.concat (<anonymous>)
    at Object.<anonymous> (C:\src\node\test\pummel\test-tls-connect-memleak.js:30:44)
    at Module._compile (module.js:602:32)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:519:32)
    at tryModuleLoad (module.js:478:12)
    at Function.Module._load (module.js:470:3)
    at Module.runMain (module.js:636:10)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
Command: C:\src\node\Release\node.exe --expose-gc C:\src\node\test\pummel\test-tls-connect-memleak.js
=== release test-tls-securepair-client ===
Path: pummel/test-tls-securepair-client
Loading 'screen' into random state -Using default temp DH parameters
ACCEPT
client connected
client: connected+secure!
client pair.cleartext.getPeerCertificate(): {"subject":{"C":"AU","ST":"Some-State","O":"Internet Widgits Pty Ltd"},"issuer":{"C":"AU","ST":"Some-State","O":"Internet Widgits Pty Ltd"},"modulus":"CFE2D764E8DC40226ADFE64A50569B8FBD68A3F7B1FD7B01705AAD0538638D3C3D095115C1F3D0429E17C2D3E2076D38BE7617C10D7F47635D4D0A8266EE3B97BF4BB2C2FFBB66B963DFB43445379D0039A9DB7ED45004D1CE687B13E29973AB4771967CACFB3E66763B4D75EC87825145CF1B953CBA68437BFE260C5E1934988A738D385AD86AB93AA04BBACB4191E167A0F41271A73A4CE59B124C60F34748E635FFA8AF74D514702F3B6B03440379607E1CB6E48C812B740973E69828D4DBB7BD0B7ABCB76EEDB058B146B663621A54134B826CC48991B0409323BDBB39B4BBFC558042CA96F8238BD126B906E305AC52298CFAC47B48A1EEF4516DD99747","exponent":"0x10001","valid_from":"Nov 16 09:32:49 2010 GMT","valid_to":"Nov 15 09:32:49 2013 GMT","fingerprint":"FF:91:92:D1:18:5D:21:9B:E2:7D:C7:9E:63:F2:51:73:A9:61:77:CA","serialNumber":"C5123AF95A7B2407","raw":{"type":"Buffer","data":[48,130,3,93,48,130,2,69,160,3,2,1,2,2,9,0,197,18,58,249,90,123,36,7,48,13,6,9,42,134,72,134,247,13,1,1,5,5,0,48,69,49,11,48,9,6,3,85,4,6,19,2,65,85,49,19,48,17,6,3,85,4,8,12,10,83,111,109,101,45,83,116,97,116,101,49,33,48,31,6,3,85,4,10,12,24,73,110,116,101,114,110,101,116,32,87,105,100,103,105,116,115,32,80,116,121,32,76,116,100,48,30,23,13,49,48,49,49,49,54,48,57,51,50,52,57,90,23,13,49,51,49,49,49,53,48,57,51,50,52,57,90,48,69,49,11,48,9,6,3,85,4,6,19,2,65,85,49,19,48,17,6,3,85,4,8,12,10,83,111,109,101,45,83,116,97,116,101,49,33,48,31,6,3,85,4,10,12,24,73,110,116,101,114,110,101,116,32,87,105,100,103,105,116,115,32,80,116,121,32,76,116,100,48,130,1,34,48,13,6,9,42,134,72,134,247,13,1,1,1,5,0,3,130,1,15,0,48,130,1,10,2,130,1,1,0,207,226,215,100,232,220,64,34,106,223,230,74,80,86,155,143,189,104,163,247,177,253,123,1,112,90,173,5,56,99,141,60,61,9,81,21,193,243,208,66,158,23,194,211,226,7,109,56,190,118,23,193,13,127,71,99,93,77,10,130,102,238,59,151,191,75,178,194,255,187,102,185,99,223,180,52,69,55,157,0,57,169,219,126,212,80,4,209,206,104,123,19,226,153,115,171,71,113,150,124,172,251,62,102,118,59,77,117,236,135,130,81,69,207,27,149,60,186,104,67,123,254,38,12,94,25,52,152,138,115,141,56,90,216,106,185,58,160,75,186,203,65,145,225,103,160,244,18,113,167,58,76,229,155,18,76,96,243,71,72,230,53,255,168,175,116,213,20,112,47,59,107,3,68,3,121,96,126,28,182,228,140,129,43,116,9,115,230,152,40,212,219,183,189,11,122,188,183,110,237,176,88,177,70,182,99,98,26,84,19,75,130,108,196,137,145,176,64,147,35,189,187,57,180,187,252,85,128,66,202,150,248,35,139,209,38,185,6,227,5,172,82,41,140,250,196,123,72,161,238,244,81,109,217,151,71,2,3,1,0,1,163,80,48,78,48,29,6,3,85,29,14,4,22,4,20,14,117,120,119,169,131,180,233,229,184,186,2,142,69,7,77,127,231,225,168,48,31,6,3,85,29,35,4,24,48,22,128,20,14,117,120,119,169,131,180,233,229,184,186,2,142,69,7,77,127,231,225,168,48,12,6,3,85,29,19,4,5,48,3,1,1,255,48,13,6,9,42,134,72,134,247,13,1,1,5,5,0,3,130,1,1,0,23,5,120,49,7,211,163,234,140,191,210,76,139,41,232,32,72,170,124,236,248,70,11,102,6,164,103,93,71,223,114,52,182,118,23,59,255,63,133,61,153,204,239,210,158,143,199,139,30,133,95,196,44,150,68,113,101,206,111,119,188,71,111,195,19,130,172,61,119,16,51,162,211,195,142,101,252,86,200,137,66,107,160,96,241,195,0,42,164,51,119,99,175,64,229,170,149,21,129,77,56,65,36,136,86,126,85,55,222,44,215,66,126,70,118,102,42,133,250,92,251,214,71,28,104,111,92,220,157,181,20,36,119,21,74,217,214,77,204,176,111,6,82,229,70,107,104,10,85,59,198,109,192,251,254,93,117,215,143,1,1,196,62,220,133,149,76,56,245,86,45,103,122,238,232,170,143,154,206,204,42,247,17,133,130,236,203,218,240,73,251,174,158,35,37,199,135,116,108,52,123,76,233,48,209,4,18,3,67,124,48,100,17,66,156,229,226,108,56,140,188,242,51,151,121,66,219,134,6,29,143,234,19,198,107,70,16,198,210,55,68,244,255,167,161,13,207,48,98,0,165,226,73,252,232,199]}}
client pair.cleartext.getCipher(): {"name":"RC4-SHA","version":"TLSv1/SSLv3"}
hello
WAIT-ACCEPT
WAIT-HELLO

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 0 == -1
    at process.<anonymous> (C:\src\node\test\pummel\test-tls-securepair-client.js:161:12)
    at emitOne (events.js:101:20)
    at process.emit (events.js:188:7)
    at process.exit (internal/process.js:147:15)
    at Timeout._onTimeout (C:\src\node\test\pummel\test-tls-securepair-client.js:91:13)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)
Command: C:\src\node\Release\node.exe C:\src\node\test\pummel\test-tls-securepair-client.js
[19:50|% 100|+ 1283|-  12]: Done
running jslint

[ghost]

@VanCoding @isaacs You might be interested in this

[Fishrock123]

Major unless proven otherwise, sorry.

1. We are probably not going to want to add another module resolution flag, is my feel.
2. Changes to the module resolution algorithm are very risky.

That is to say, at least, do not expect when/if it will land but we appreciate the effort and care about it nonetheless.

[ghost]

@Fishrock123 Researching this will require participation from the ecosystem. Expecting them to git clone a fork/branch from an unknown org, and then build a version of node themselves is a lot to ask.

Given how simple (and easily reviewable) the changes are, and that by simply including the PR the default behavior of node does not change (an can be so easily validated), I'm incredibly upset, and generally disappointed, that you would take such and adamant stance against helping make node better.

Both of your concerns I have spoken to and addressed in the description, and leave me with the general impression that you are simply ignoring the details.

How unfortunate for those of us who use node on a daily basis.

[Fishrock123]

an can be so easily validated

This is mistaken, the tests unfortunately do not cover all of what is possible via the APIs.

---

Sorry if I came off wrong, I wanted to let you know the likely response upfront. I don't have time to read it right now and am mostly out of the conversation other than that landing of this needs extra review. The module system is "locked" with reason, although it is possible for changes to happen.

[Fishrock123]

Understanding and in respect of your esteemed position

... pardon? Any other collaborators here have about as much say as I do.

that caused you to decide it was not deserving of your valuable review and thoughtful consideration

I know little about how the module system works. I, myself specifically, have other more pressing things to do at this time. Maybe others will chime in (American thanksgiving is this week so people may be busy).

Long explanations attempting to... I don't even know what about my position will not help anyone else read this thread. You best course of action, was, is, and usually will be to wait a bit on large issues.

[ghost]

As I said I'm new to this so my sincere apologies; I was only wanting to better understand you and your statements and this process.

Github indicates that you are a Node.js Foundation Member, and I was attempting to honor that. I was also attempting to respect your time and had assumed you had important things to be doing as we all do. But it came across as if you had no intention of spending any time, now or in the future, reviewing the PR, and I was wanting to understand what about it may have lead you to that decision, so that I might better handle how others might come to the same place.

I understand it will take time for this to be reviewed and so forth, but again it came across in a way that implied it might never come to that, and given what I thought was your position I reasoned others might give weight to your words and conclude this PR would never come to be.

My sincerest apologies as I detect I've irritated you.

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/450/

[ghost]

@Trott That was incredibly kind of you. Thank you.

[ghost]

@Trott I'm presuming you're running the tests with the switch off, to verify default behavior has not been altered? I hope that will help get the PR merged someday.

What would also be interesting is to run with the switch on. Is it relatively straight forward to run the tests, but ensuring every spawned process have an environment variable set?

[ghost]

@Trott Thanks again for running those tests on nodejs's machines.

I'm looking at the early results and the tests seem to be failing for reasons such as node's version being 8.0.0-pre, and not necessarily from a failure in the Modules subsystem, so I'm wondering if there's a baseline of some kind to distinguish between failures in the 8.0.0-pre master and those as a result of the PR?

[Trott]

@Trott I'm presuming you're running the tests with the switch off, to verify default behavior has not been altered?

Yes.

What would also be interesting is to run with the switch on. Is it relatively straight forward to run the tests, but ensuring every spawned process have an environment variable set?

I don't use that CI job too much. Let me @-mention someone who uses it a lot and see what they have to say: @thealphanerd

[ghost]

Think I've scared everyone away, probably before they even got here.

Anyways, I've been developing this PR on Windows. In getting citgm running on a linux vm, I ran with the PR switch on, and hit the first issue. This is actually an issue in --preserve-simlinks as well, and answered the question I had as to why --preserve-symlinks didn't preserve the symlink directory of entry.js.

In my testing, --preserve-symlinks not preserving the entry.js directory caused problems when npm would attempt to run node scripts that had been installed down into /node_modules and were in a directory that had been symlinked. The PR changed that so the entry.js directory was always preserved.

This worked fine on Windows, and I could also still run globally installed commands. However, on linux when trying to run a global command, like npm, it failed with the PR active. This is because on linux, npm creates an executable filetype-symlink directly to the .js file, which has the #!/usr/bin/env node as the first line. But on Windows, there is no filetype-symlink, rather npm creates a .cmd batch file that launches node with the particular entry.js.

You have follow the symlink for the entry.js file, or it doesn't work on linux.

So, probably how --preserve-symlinks should be changed, and how I will change the PR, is to only follow symlinks for the entry.js's directory, if the entry.js file is itself is NOT a filetype-symlink. Otherwise, the directory of the entry.js must be preserved. Shucks...

[Trott]

@phestermcs I don't want to discourage you on this. You've done a chunk of work and you clearly feel strongly about it and have thought about it a lot. But there are big obstacles for this. It doesn't mean it's never going to happen. But it does mean it might never happen and, if it does happen, you will need to be patient and persistent.

The big obstacles are:

• Module loading is (obviously) a very dangerous area where even harmless-seeming changes surprisingly break things for very large numbers of people. The module API is locked, which means that the bar is very high to introduce changes. Before any new features can be introduced for the API, there must first be agreement that the API should be unlocked. That's a really huge step that the project does not take lightly.

• Adding undocumented and experimental features is great in theory, but in practice, people find them, people start using them, they find their way into StackOverflow answers, and then we can't remove them without breaking a large part of the ecosystem. And then we're stuck with it, even if it only half-works and no one is interested in maintaining or improving it. This is not a hypothetical problem. So, again, this is not something the project does lightly.

Therefore:

• First, the case needs to be made that there is a problem of enough significance that the API should be unlocked. Asking for an undocumented feature to be landed before making that case is not going to work.

• Unlocking the modules API is sure to be hugely controversial and therefore almost certainly something that cannot be decided other than through @nodejs/ctc approval. So expect this to be a lengthy discussion with a lot of process overhead. It will need time and you will probably need to end up convincing a few people individually.

If you want to see this (or something similar to it) land, here's how I suggest you proceed:

• Create a markdown gist or other document that succinctly (or as succinctly as possible) describes the problem this solves. Markdown is a good choice IMO because if you are asked to submit it as a Node EP (enhancement proposal, see https://github.com/nodejs/node-eps) or something like that, you can do a lot of copy/paste. With all due respect, and I say this as someone who does the same exact thing (and is doing it right now!), you have a tendency to use quite a few more words than necessary to describe a situation. If you can edit something down, it may be more compelling (or more likely to be read!). Even if you can't make the whole story short, maybe at least condense a handful of bullet points that are then explained further in the lengthier text?

• Be prepared for a long process. Chances of this getting implemented as an experimental undocumented feature anytime in the next week is effectively zero. You have to be thinking in terms of months at least.

• Get people in core excited about this, probably by creating that concise .md file I describe above. Nobody is going to spend a lot of time reviewing code or reading lengthy issues if there's not a sense that it does something important. Nobody is going to want to change module loading if the ecosystem improvement is anything less than enormous.

• Be prepared for skepticism and for answering the same questions many times.

Don't be surprised if you work hard to advocate for this and it doesn't happen anyway, because you are definitely facing uphill on this. Again, that's not to say it absolutely won't happen. But it is to say that the burden of convincing a lot of people that this is worth doing is on you (and anyone else who wishes to advocate for it). I'm personally unconvinced (at least as of this moment) that this is something we want to do, but I wouldn't say that I'm impossible to convince. (I'll also admit that I haven't yet read yarnpkg/yarn#1761 and probably other related things that may or may not affect my opinion.)

[mscdex]

FWIW I think a good tl;dr would be good to have at the top of the OP. That is a lot of text to take in ...

[Fishrock123]

An EP may be most appropriate here, I agree. It is great to have working code to go along with it though.

[ChALkeR]

Not commenting the nature of this PR, this change doesn't look semver-major to me.

All code paths that are added are wrapped in if (adjacentNodeModules) { which is off by default and is supposed to be enabled though a runtime flag. There are also minor code movements, e.g. NativeModule.nonInternalExists(filename) is now called when it wasn't called before (it's now called before one check that could return, when it was called after that check), but that could be easily eradicated.

If the new flag isn't switched on (it's not by default), the behaviour is (or should be after minor tweaks to this PR) completely unaffected. Note: I mean the actual observed behaviour. Timings, optimizations, etc. could be (or not be) significantly affected by this PR.

Another question is what happens when the flag is switched on and how far does this break user expectations, and how far would enabling this flag break existing ecosystem packages — that I can't estimate atm.

[ghost]

Thanks

@Trott thank you so kindly for replying so thoughtfully and providing actionable feedback; your time in doing so is greatly appreciated. I agree and understand with everything you say, and I'm fully prepared to push the rock uphill.. I was just getting the impression over the last month or so, that no one would be at the top of the hill to actually take an honest look at the rock as something that annihilates a real problem.

Regarding risking the eventual need to support an undocumented feature, totally get it. It's just one way to get bits into peoples hands in order to collect empirical real-world evidence; maybe, after I've convinced you of the value and merits of the thing, you can help in the other ways.

TL;DR

My sincere apologies to @mscdex, @Fishrock123, and any others trying to even contemplate reading the PR comment because of its length; I've spent more time trying to succinctly define the essence of the thing than actually fixing the thing, because this issue in particular has a number of unique challenges:

• It's dealing with filesystem linking, which can seem a bit mysterious and confusing at times all by itself
• There's been failed attempts both within node and package managers, so there's a bad taste in people's mouths for trying again
• Fixing it requires changing a locked subsystem in node
• The claims I'm making about my changes may seem almost fantastical in the light of those things
• The changes themselves are exceptionally nuanced
• I'm an entirely unknown quantity

I've found it extremely difficult to engage with anyone at a deep technical level about any of this, and practically all input as been anecdotal, like 'symlinks have broken vast amounts of the ecosystem', with adjacent yet unrelated issues thrown in, while actual technical descriptions of actual problems have been few and far between. Frustrating to say the least.

The only thing I ask, is to please grill the ever living s__t out of me about all of this; give me an honest shot to convince you! And when I have branches of node and yarn showing it all working, get my fork/branches, do the build, it see it all working for yourself! But just give me an honest shot, and I won't let you down, even if that means the whole idea needs to be s__tcanned; despite how it may seem, I am very reasonable person.

I love node guys, and its influence, you just can't know how deeply I do; if only you knew my background and experience. I want to make it better for everyone and to keep it moving forward.

Just imagine for one second

This PR will, once and for all, give us symlinks that just work, will be backwards compatible, and will give package managers the option to store all module@versions once on a machine, and symlink to from anywhere on the machine. Memory bloat and addon crashing be damned. I would even take the bet, that within 2 years, the --preserve-symlinks flag will be gone, and this PR will be the default way node works.

I'm currently running citgm on baseline v7.1.0, a v7.1.0 with PR but switch 'off' and, then with switch 'on', and all results are identical. This means, at least from citgm's perspective, with switch off the default behavior is preserved, but what's most exciting is that with switch on it's fully backwards compatible!!

I'm going to test the hell out of this for sure, but the initial results look quite promising.

Just give me a fair and reasoned shot, please.

[ghost]

@ChALkeR Thanks kindly for actually taking a look at the code.

I've been doing dev on Windows. I just started last night testing on 'nix, and it did uncover an issue. I've sense coded for and I'm currently testing (with great success btw), but unfortunately it's just a smidge more intrusive, and will be more controversial.. such is life

[ghost]

...and not to ruffle any feathers... well, not too much anyways... there's a reason nodejs started using even/odd semver-majors no? and looking at commit history there's been over 20 commits involving modules.js in last year, ~6 in last 3 months.... just sayin' ;)

[ghost]

Can someone tell me exactly what would be the best format of citgm output, or best way to present comparison of results showing no difference between versions?

[ghost]

@Trott Maybe you can answer two simple questions:

1. Should node fully support symlinks so they're transparent to things that shouldn't care?

My opinion is either don't support them at all, or fully support them. Right now I would say node does neither, but sits quite poorly in the middle, which I think is in many ways worse than either, because predictable and understandable default behavior goes a long way to creating high-quality, bullet-proof software.

2. If node could fully support symlinks and be completely backwards compatible, should it?

Symlinks are a very valuable way to optimize use of the file system for certain classes of problems, and if ever there was a real world example of the kinds of problems they best solve, it's node's modules.

If your answer to both those questions is yes (and why wouldn't it be), then I've already convinced you this PR is of substantial value; I just now have to show you this solution works beautifully. (I'm not even going to pretend you answered no ;) )

[VanCoding]

@phestermcs Why did you close this PR?

I finally took the time to read your proposal, and I think it's pretty smart!
But I think you should have written the "Simultaneously Preserving and Following Symlinks" section before the "Adjacent Node Modules" section.

The reason for this is, that your proposed "Adjacent Node Modules" would not work without changing how symlinks curently work. But changing symlinks to behave like you proposed, does not require to also implement adjacent modules. This made it a bit harder for me to understand it.

But now that I understand it, I think it's awesome!
Please guys, let's talk about it seriously!

[ghost]

@VanCoding Thanks kindly for taking the time :)

I created the PR primarily to begin a discussion. I'm still learning how best to do these types of things in nodejs without annoying everyone with long prose, to the point they dismiss out-of-hand a good idea simply because of how it was presented and from whom it came blush.

uhm... maybe TL;DR??

You are correct, fixing --preserve-symlinks should have come first, and in fact I'm in the process of rejiggering some test branches, but I kinda back-ended into it having first started with adj-nm. There's also an additional fix in --preserve-symlinks to only follow main.js if it's a file-symlink, and not when it's just a file potentially through a directory symlink (this was breaking tooling).

Fwiw, adj-nm stills 'works' without using symlinks, but it enables using symlinks to a machine store of modules, so they 'kinda' go hand-in-hand.

What I'll have later today in my fork:

• branch fix-preserve-symlinks that just has those fixes, based from master
• branch v7.2.0-fix-preserve-symlinks that I will run through citgm with switch on and off, to demonstrate nothing breaks with off, and that it works way better when on; I've already tested this with citgm and its awesome.
• branch enh-adj-nm that is adjacent-node-modules enhancement based off fix-preserve-symlinks. It has:
  • NODE_ADJACENT_NODE_MODULES switch to enable just that, so I can prove via citgm that it's fully backwards compatible, FULLY, both with switch on and off.
  • NODE_SUPPORT_SYMLINKS switch to turn both --preserve-symlinks and adjacent-node-modules on.
  • unlike --preserve-symlinks, these can only be set via env-vars, because it needs to be all or nothing when running things like npm that can end up spawning scripts that call node (that don't have the switch on their command line, and shouldn't)
• branch v7.2.0-enh-adj-nm to run through citgm in various config to prove things work.

I don't really intend any of these to ever be directly pulled by nodejs, but just to be used by us brave few for testing purposes.

I'm going to 'publish' all this as an issue titled 'fixing --preserve-symlinks', rather than a PR, with things explained in simple bullets so it will be 10x's shorter than the description in this PR. It will include links to my fork/branches so others can clone and test, and links to gists showing all results from citgm.

Stay tuned.

[VanCoding]

That's a good plan!

1. Try to make the symlink-fix happen
2. Try to make adjacent modules happen
3. Convince package-managers to make use of the above 2.

[ghost]

We're thinking alike :). I've looked quite a bit at npm, yarn, ied, and pnpm. I will be creating issues on all their repo's to pull them into the loop, hopefully get their support.

I love node, a lot. It lets me run super fast, except with a thorn in the ball of my foot. Hopefully someday we can run without the thorn (and have installs take like 1 second)

[ghost]

@VanCoding

• it's working great, better than great!
• citgm test results between v7.2.0 and v7.2.0-sjw identical.
  • citgm is not fun to use, can't be scaled at all (yet)
  • FunFact! citgm uses the version of node being tested to run itself! (gulp)
• made simple tool to quickly laydown directory/mod structure of any kind from json, and gen mod src
• tool clearly shows require (linking) & access (fn() call) graphs, with following info
  • require alias [dep]
  • module name [dep@v1]
  • _dirname and realpath
• used to create tests of all permutations of dependency resolution
  • using old (mod/node_modules), showing fully backwards compatible
  • using new (mod.node_modules), and with symlinks to machine store
  • interop of both (i.e. showing modules with 'bundled' deps still work)
  • layouts like npm and yarn, bubbling dependencies, using machine store
  • layouts like ied and pnpm with everything flattened, using machine store
  • stores can still be local to tree, or mix of local, machine, with both symlinks and copies
• repo'd memory bloat, and can show that no longer happens
• still wanting someone to provide an add-on that crashed from multi-loading, but I can make one
• finishing tests and collecting results, and will post issue with info & links tomorrow
• will then complete fork/branch of npm POC that stores modules to machine store
  • initial tests: 2 min installs take only 2 seconds... 2 seconds!!

Are you able to, or would be willing to build my fork/branch and see how it works for you?

[ghost]

Please offer both myself and symlinks a second chance to make a first impression with this new issue