Fixing --preserve-symlinks. Enhancing node to exploit.

[ghost]

A working prototype that enables using symlinks to machine level module stores, so modules no longer need to be physically copied and duplicated wherever they're used on a given machine. This is accomplished by:

1. Effectively, setting __dirname of the "main" entry module to the preserved path passed on the command line, or if it's a file-symlink, using its target path. This is intended to start the program off in the right symbolic path space, to address tooling problems resulting from the current behavior, where __dirname is always the fs.realpath().
2. For all other require()d modules, always using the preserved path of its directory location as its __dirname, but always using its realpath as the module cache key. This resolves the memory bloat and add-on crashing problems while still letting resolution happen within the symbolic path space.
3. When building the list of search paths to be used for resolution, by interleaving adjacent somemod+node_modules directory paths after the subordinate somemod/node_modules, the directory structure dictating dependency version resolution can still be bound and specific to a given top-level /node_modules, but can now be completely decoupled from the physical directories of the modules involved.

[zkochan]

This is great! We need it!

cc @alexanderGugel, @wmhilton, @iamstarkov, @rstacruz

[zkochan]

I think the naming of the adjacent node_modules should not be a valid npm package name. Someone can publish a package named foo.node_modules. The delimiter should probably be some character that is not valid inside a package name. Like a + or @

[zkochan]

+ and @ are not valid. Also the space, +, =, ~, &, $, #, ^, ;, comma are not valid. You can check with this package: https://npmjs.org/package/validate-npm-package-name. (There are more characters that are not valid but I listed only the ones that are valid directory names).

[zkochan]

I don't insist but this is easy to change and I think solves an edge case.

[zkochan]

What about instead of foo`node_modules just use foo`s. It has some semantic sense.

[ghost]

@zkochan Thought a little more, and I think the + character is the better choice. Both ` and ! are operators is nix shells.

So I'm just going to change that tonight.

[joshgav]

@phestermcs the patterns and semantics for file locations and behaviors you're defining in this thread would I think fit perfectly with work in the version-management discussion group, see e.g. nodejs/version-management#3 and nodejs/version-management#12.

/cc @jasongin

[ghost]

@joshgav Reading the discussions on the repo, they seem to be more about managing multiple versions of node proper, than modules used as part of a javascript program that is hosted by node. The only applicability might be, should nodejs/node some day adopt the adjacent node modules functionality in this issue, that package managers might benefit from a standardized way of positioning machine stores as node-environment stores, that are aware of "root pathing" for a given node environment.

Our discussion here is merely about what specific character to use to distinguish a directory name, and it appears given what can be used and prior convention, the one best character is the + symbol, as in '/mymodule+node_modules/'.

Or am I completely missing something?

[joshgav]

@phestermcs

Our discussion here is merely about what specific character to use to distinguish a directory name

I see 💡 . Okay, well at least that group is now aware of your concerns too. Thanks!

[isaacs]

There's a lot of pitching going on in this thread, but is there a succinct explanation of what the proposed changes are?

Could you rebase your branch onto master so that it would be easier to review the changes? I'm not sure what is being proposed, or how it differs from discussions in the past around handling symbolic links in the module loader.

[jasnell]

@phestermcs ... first off, thank you for the effort that you've put into this. Given how critical the module system is to Node.js, we will have to carefully consider the change that you are proposing. The preferred approach for something like this would be to open a enhancement proposal in the https://github.com/nodejs/node-eps repository that expands on the detail that you have provided here. Ideally that proposal would evolve into a pull request that we can evaluate and review more directly. Backwards compatibility is going to be the most significant factor when reviewing this so please keep that in mind.

[jasnell]

There are two things we need: The EPS and a pull request. This is unlikely to get significant review until either/both of those arrive.

[jasnell]

I'd start by simply copying the text of your original post into the EPS and seeing where the discussion leads. As I have the opportunity over the coming couple of weeks I'll try to dig in on this to start reviewing.

[isaacs]

@isaacs Where the descriptions in the How section not succinct enough?

No, they are not, that's what I'm saying. The superlatives and format changes are very distracting. I'd just look at a set of diffs, but since it's not based off master, I'm unsure that what I'm seeing is actually the changes being suggested.

The links to code in the How section are to isolated snippets, which are challenging to put in context. I'd love to just see a full diff of the changes being proposed, and a very terse explanation of the current behavior and the technical changes. Use as few words as possible, and just say what it is, not why it's the greatest thing ever.

[jasnell]

Deep breath everyone. Let's not waste time hand wringing over communication styles and focus on the code, shall we?

@phestermcs, thank you for the PR. It is much appreciated. It will take a couple weeks, I think, to do a proper review so I ask for your patience. :)

[jasnell]

Absolutely reasonable! I see @mscdex has already jumped in to handle some of the style nits. I'll continue discussion over in the PR!

[addaleax]

@phestermcs I get the feeling that you are expecting more backlash here than you’ll actually receive, and try to answer criticism for your PR before that is even voiced. In any case, it’s a plus to see that you’ve thought this through.

Your PR isn’t huge (which is a good sign), and we’ll get to review it and talk about it. I think I’m pretty much agreeing with Isaac here, so maybe it helps if I try to rephrase what he’s saying:
It helps others to understand your proposed changes by seeing how you implement your changes more than seeing why you do that. It’s hard to give feedback on bullet point lists of motivations for a feature, it’s much easier to give feedback on the changes to the actual code behind a feature.

I can assure you that you have offended neither @isaacs nor @thealphanerd. They are both people who I fully trust to give everybody a fair treatment, and I would read the comment I believe you are referring to as asking for a better way for us to understand your perspective.

(Also, another tiny tip: Using @mentions in commit messages is always a bit confusing because the mentioned person receives notifications from a repo that they don’t usually interact with. EDIT: I see James already mentioned that :))

[jasnell]

All PRs for new changes are made against master and must land there first. From there we will cherry-pick changes back to the release branches. We do no accept changes to release branches that have not yet landed in master

[isaacs]

@phestermcs You haven't offended me, but I appreciate your concern and eagerness to be respectful, and your kind words about my software development expertise. I'm often slow to respond because I run a startup and have a young baby, and both things eat into my OSS time. Reviewing your summary now.

[billiegoose]

This is certainly exciting!! Looking at the changes now.

[Trott]

@phestermcs TL;DR is typically short hand, roughly, for "If the rest of this is TOO LONG and so you DIDN'T READ it, here's a short summary. If you only read one thing, make it this little bit." And then you try to sum things up in a sentence or two.

You seem to use it to precede things that are long and detailed instead.

I don't (I hope) normally nit-pick about such things in a lengthy issue tracker conversation, but every time you've used TL;DR, I've been confused because it is never followed by any thing brief. And it had me thinking that you simply could not summarize things and puzzling over how to make communication effective. Now I realize that it's just you're using the acronym in a way I'm not used to seeing it.

So above, where you have "Succinctly?" That's a TL;DR (at least in my experience). Below, where you have TL;DR? That's more like "All The Details" or something (again, in my experience at least).

[ghost]

ftr. i removed several comments as they were irrelevant to the issue and inappropriate.