Fix repeated execPath directory

[ehmicky]

This is another implementation for #3 since that PR became stale and does not include tests.

This might solve the following issues on execa:

• execa does not respect $PATH variable execa#153
• preferLocal prevents local from working execa#196

npm-run-path unshifts dirname(process.execPath) to the $PATH. From my understanding (correct me if I'm wrong) the reason is this:

• a user might have several versions of node installed
• a specific version of node different from the default one (i.e. not in $PATH) might be the one currently running
• we want to ensure if node gets called in a child process, the same Node version will be used.
• arguably we also want globally installed binaries to be the ones installed under that Node version.

Now I am wondering how this would happen in practice. For example, nvm use and nvm exec correctly modify the $PATH, so this is not a problem with nvm. @sindresorhus maybe you have more information on why this is needed?

Now the problem is that this conflicts with other tools that are competing for priority in the $PATH.

I am not sure whether firing the right node version or giving priority to those other tools is a decidable problem with an objective solution? But at least what we could do is not do anything if dirname(process.execPath) is already present in the $PATH, which this PR implements.

Current limitation: this does not work if dirname(process.execPath) is symlinked and referenced under different symlink names under both the $PATH and process.execPath. This is harder to fix since the current code is synchronous and relies on path.*() not fs.*().

[arcanis]

While it will fix my own particular case, I still have concerns about doing this in the first place. If I call a Node binary through its direct path (assuming that this binary isn't in the PATH, for example if I want to call it directly from its nvm location), my local binaries will end up being ignored.

(That said, I think this change is the safest thing that could go in a minor so it's fine by me to land it - I'd just ask to reconsider this behavior being the default in the next major)

[ehmicky]

@arcanis If by "local binaries" you mean the binaries inside node_modules/.bin/, then this should not be the case. npm-run-path unshifts both node_modules/.bin and dirname(process.execPath) to the $PATH, but node_modules/.bin is unshifted last (i.e. it has priority).

We are working on a major new execa version, so breaking changes are an option.

[arcanis]

Yep, unfortunately in the case of Yarn we don't use node_modules for local binaries (they are stored in the temporary directory described in sindresorhus/execa#196), so this heuristic doesn't work.

The .bin folder is a bit of an implementation detail, and rather than assume its existence the real fixes should have been implemented by us, the package managers. This is what we've eventually made, but a bit late and thus it conflicts with custom-made approaches like this one 🙁

[sindresorhus]

a user might have several versions of node installed
a specific version of node different from the default one (i.e. not in $PATH) might be the one currently running
we want to ensure if node gets called in a child process, the same Node version will be used.
arguably we also want globally installed binaries to be the ones installed under that Node version.

Correct

Now I am wondering how this would happen in practice. For example, nvm use and nvm exec correctly modify the $PATH, so this is not a problem with nvm. @sindresorhus maybe you have more information on why this is needed?

It's definitely needed. I don't remember exactly all the reasons, but even npm is using this technique. While nvm can change the $PATH, there are many scenarios where it doesn't do it correctly. We set this to ensure the correct node is always consistently found no matter what tools are used.

[ehmicky]

This makes sense.

Now it seems to me the core problem is that several tools are competing for priority in the $PATH: execa, yarn and version managers.

Although this is more of a workaround, this PR might solve the problem for yarn and other users. Should we merge it?

[sindresorhus]

If we merge this, it means process.execPath is not always first, so there could be a situation where execa uses a different Node.js binary, maybe even the wrong one that doesn't work because it's old. I would really like for us to be able to guarantee in execa that if you execute a script that uses Node.js, you're sure it's using the same Node.js binary. This can potentially prevent so many problems.

[ehmicky]

Yes I see what you mean. There should be no real cases where a user would want to use a different Node.js binary in the top process and the child processes. I.e. enforcing the same node executable is used seems like a good choice to me as well.

On the other hand, as a side effect, all the binaries inside the same directory as the current node binary will also get a higher priority. This side effect is creating issues:

• for execa does not respect $PATH variable execa#153, the user has git installed in two different directories. This makes execa pick the wrong one.
• for preferLocal prevents local from working execa#196, the user has yarn installed in three different directories. This makes exec pick the wrong one, which makes yarn policies set-version not work.

Is there a way to solve the issues created by this side effect, without requiring users to use preferLocal: false?

[arcanis]

There should be no real cases where a user would want to use a different Node.js binary in the top process and the child processes. I.e. enforcing the same node executable is used seems like a good choice to me as well.

In the general case maybe, but those kind of assumptions are bound to break as soon as you go on slightly different setups. We have two real-life examples (nvm / yarn), but you could imagine a Node project being bundled all-in-one with something like zeit/pkg (which bundles its own Node binary) - in this case you don't necessarily want the same Node to be used for your spawned process; you would want to use the global one instead.

Now you might say that users can just toggle the feature off if they don't need it, but the fact is that they won't because 1/ they don't know what are the drawbacks of this option 2/ they tend to not even be aware of this option in the first place - they just use execa as a promisified child_process.

Hence why the default behavior should be the safest and most expected one imo.

[ehmicky]

Trying to resolve the deadlock, I think the way forward might be to a boolean option useCurrentNode to turn on/off the execPath behavior:

• defaults to true.
• add that option to execa as well, merely forwarding it to this module.

Like this execa keeps the current behavior when it comes to execPath, but users like @arcanis and @lucasbraun can fix their specific setup.

What do you think?

[arcanis]

Unless the default is false I don't think this would work. The problem isn't that execa doesn't work in my case. It's that execa corrupts the PATH when installed through Yarn. All execa consumers are affected by this logic, so it cannot be fixed in an option 🙁

Counterproposal: do not add the node path to the PATH if Yarn is detected in the project root (ie if yarn.lock exists).

[ehmicky]

@arcanis Based on @sindresorhus comment above, a solution where execPath is never pushed will most likely not get merged, and this PR will just stall. I'm proposing a compromise to make this move forward.

Also this issue is not only about Yarn. sindresorhus/execa#153 is affecting by it as well.

I.e. a way to move forward might be:

1. add a boolean option defaulting to true
2. if this is merged, continue the discussion on preferLocal prevents local from working execa#196 about whether that option should be set to false when we detect Yarn. Not that this separate fix would need to be done in execa not in npm-run-path since npm-run-path is synchronous and does not rely on fs.*.

[tunnckoCore]

I probably don't get it. Sorry if I'm missing something but believe that we can fix that. But before few months created a package (all-module-paths) for a similar thing. Not npm-run-path, but around the case that I needed all possible bin directories, (except the system-wide), also be able to work with nvm, without nvm, yarn globally installed binaries, all the node_modules/.bin dirs up to the home and etc.

Disclaimer: THAT'S NOT A PROMOTION, please. 😺 I've done that package, exactly because I needed a way to make package.json scripts run better and find & run automatically what I need, or at least make package scripts work better for mono-repo scenarios. This whole thing is used inside the @tunnckocore/scripts which basically can run everything you want and truly can be used as yarn exec or npm exec command - it uses execa of course.

It was also because global-modules didn't support yarn global isntalled bins.

The thing is that I don't even use execPath, I collect all dirs in the following order (first item -> last item in array, first ones take precedence, right?):

1. ./node_modules/.bin - we always need locals first? if there is another case, okay no problem all-module-paths gives you 2 groups - globalModules (e.g. nvm and yarn) and localModules (e.g. all the node_modules/.bin up to homedir), so you can construct the path or reverse the arrays and etc.
2. and all node bin dirs up to the HOME,
3. and all the other global paths like the currently running nvm node (e.g. ~/.nvm/versions/node/v8.16.0/bin), pnpm global bin and yarn's global bin.

Then just prepend them on $PATH.

(see the source code of all-module-paths)

So, example usage

const mod = require('module');
const proc = require('process');

// which uses `global-dirs` (by @jonschlinkert) under the hood
const allModulesPaths = require('all-module-paths');

const paths = mod._nodeModulePaths(proc.cwd());

// see the screenshots below
const dirs = allModulesPaths({ paths });

console.log(dirs.allPaths.binaries)
// => binaries: 
//       [ '/home/charlike/dev/foo-bar/node_modules/.bin',
//         '/home/charlike/dev/node_modules/.bin',
//         '/home/charlike/node_modules/.bin',
//         '/home/charlike/.node_modules/.bin',
//         '/home/charlike/.node_libraries/.bin',
//         '/home/charlike/.nvm/versions/node/v8.16.0/bin',
//         '/home/charlike/.config/yarn/global/node_modules/.bin' ]

const PATH = dirs.allPaths.binaries.join(':');

proc.env.PATH = `${PATH}:${process.env.PATH}`;

And that's it, it works flawlessly. I don't get why we should care and mess with the priorities inside the already given to us $PATH - we just prepend the javascript ecosystem to have priority over the system-wide binaries.

So, to now we have everything, except the node bin itself, right? So add it at the very first place?

Few pics:

[ehmicky]

Thanks for your input @tunnckoCore! Maybe @sindresorhus will have a different take of it, but in my opinion this solution looks a little complicated and I'm wondering if this might not add extra complexity to the original problem.

I think this could deserve a separate PR though with no code in the PR nor source code screenshot or implementation, but just a clear description of the algorithm.

[tunnckoCore]

The thing is that I'm not exactly sure what's the problem. From the discussions around the issues here and in execa, I'm understanding that the question is where exactly to be the execPath, right? To be in the very first place or somewhere else in the middle? I might need to look on the execa source code again to remind myself what is happening there too.

[ehmicky]

I'm having a similar problem as @arcanis in one of my projects using execa.

nve spawns a command with a specific Node.js version (similar to nvm exec):

$ node --version
12.11.1
$ nve 8.0.0 node --version
8.0.0

It is a thin wrapper around execa. The wrapper is called with the user's current Node.js version (12.11.1 above) but execa is called with the Node.js version specified as argument (8.0.0 above). nve modifies the PATH so that child processes keep using that Node.js version (8.0.0 above).

However that does not work with preferLocal: true because npm-run-path pushes the execPath (12.11.1 above) back to the front of PATH.

Possible workarounds in my case:

• using preferLocal: false and passing the env option to execa after using npm-run-path and prepending the right Node.js executable to the PATH. This is the solution I've picked.
• copy/pasting the code from npm-run-path but without that line of code related to execPath.
• modifying process.execPath to the right Node.js executable. This definitely feels hacky.

However I think it would be better to:

• provide an execPath option that defaults to process.execPath. This would be a non-breaking change since it defaults to the current behavior.
• make execPath have higher priority than node_modules/.bin. Otherwise if npm install node has been installed, that node will be used instead of execPath. This can happen if npx -r [email protected] ... was previously run. This would be a bug fix.

[ehmicky]

I am closing this in favor of the following PR: #8 and #9.