feature request: option to add node_modules to the path for shelljs scripts

[nfischer]

This is a feature request to add ./node_modules/.bin/ to the $PATH for ShellJS scripts, which would change the behavior for the shell.exec() function.

This is something offered by shelljs-nodecli, and I see no reason why this couldn't be part of ShellJS-proper. This is very useful for writing a shelljs script that takes advantage of a binary installed by a dependency, such as jshint:

shell.exec('jshint .');
// instead of:
shell.exec('./node_modules/.bin/jshint .');

I see this as being useful in writing gulpfiles or something of that sort.

This could either be the default behavior or not, and could be exposed as a boolean option such as config.nodepath = true. I'm open to naming suggestions for the actual option.

[ariporad]

Sounds like a great idea!

[gyandeeps]

I think this should be an option for exec command only, thoughts?

[nfischer]

We're also adding a new exec-replacement in #524, so the option should affect that command as well.

If this were implemented as a global option, this would only actually have an effect on exec, cmd (the exec-replacement), and which (since it modifies PATH).

I'm also thinking of this as it would relate to shx. It might be easier to expose it as a global ShellJS option, since that could be enabled via shx --nodepath cmd eslint *.js (assuming we extend shx to support exec or cmd). This would behave just like eslint *.js except that it would support globbing in npm scripts on Windows as well as unix (see shelljs/shx#68).

[gyandeeps]

i have this working but its tricky to test. Since when you run npm test to run the unit test. NPM already adds the ./node_modules/.bin to the $PATH. 😄
Can i pull in sinon to mock process.env.path stuff?

[nfischer]

Sure. It's probably easier to just remove it from the PATH for the sake of the test. Bringing in a new dependency for a single case probably isn't worth it if there's a good alternative. But if this is the best option, I don't see a reason why not.

We might even consider leaving this to users that want this feature. They can modify process.env.PATH as easily as we can, and I think exec already takes an env argument.

Or we consider instead offering a cross-platform way to easily add to the PATH (so users won't have to worry about path separators, back vs. forward slashes, etc.). There are a few directions we could consider taking this.

[nfischer]

After some thought, this is probably the wrong approach to solve this problem. I think it's better to leave this to users, since it can easily be solved outside of ShellJS.

[emulvihill]

Too bad, this is a real bummer when you have to give your codebase to someone else. They either have to add all the tools into their globally installed packages or else have the magic path entries. I always try to make my projects as easy as "git clone, npm install, (do stuff)" Things will fail if I can't properly use exec.

[nfischer]

@emulvihill It sounds like what you're looking for are npm scripts (see the part about "arbitrary scripts"). No need to modify your path or add things globally, you can do things like npm run foo.

---

If you really need shell.exec() to run local binaries as if they're in your PATH, you can do this:

process.env.PATH += (path.delimiter + path.join(process.cwd(), 'node_modules', '.bin'));
shell.exec('ava --serial test.js');

[freitagbr]

This is correct, npm adds the node_modules/.bin/ of your local project to your $PATH when you run npm scripts, and you can combine and compose them.

[emulvihill]

Thanks for the suggestions! I don't like NPM scripts for such purpose because they are not platform independent without a lot of ugly code. I did solve my problem by using the shelljs-nodecli (as was mentioned in the original issue).

[nfischer]

I don't like NPM scripts for such purpose because they are not platform independent without a lot of ugly code.

Huh? Have you taken a look at this blog post? See "Misconception #4: npm Scripts Don’t Run Cross-platform"

I did solve my problem by using the shelljs-nodecli (as was mentioned in the original issue).

I recommend against that package (unless you know what you're doing) because:

1. The 2-liner I posted gives you all the benefits you're looking for. No extra module necessary
2. shelljs-nodecli hasn't had a patch since 2014 (still uses ShellJS v0.2). We've fixed tons of bugs since then, including cross-platform bugs, so there's real benefit to using ShellJS proper.

[emulvihill]

Haha, yes I did see that blog post. I ARDENTLY disagree that it the code that you recommended OR that blog post is good practice, that is the ugly hack I was referring to. Not trying to argue, I just think it was a shame that you did not implement the original suggestion which would have abstracted the pain away from the developer. With NPM scripts, it is NON-cross platform by default. Every junior developer on a team will have to consciously WRITE cross-platform hacks, or else the code that "worked on their box" will NOT work on someone elses. This is why gulp is so great, you need to go OUT of your way to write bad code.

Again, sorry to keep arguing on this issue but that blog post was wrong-headed and will cause people pain on different platforms if they don't use tools that take care of platform differences behind the scenes.

[conartist6]

Oh wow this has been open since 2016 and it's still hung up. I installed shelljs and intended to use it, but I've changed my mind. I was also depressed to find I'd still have to use cross-env to set an environment variable. Back to execa! Windows can rot, good riddance.

[conartist6]

Since it I don't see it linked I did find #866, but while the PR is merged the functionality is still unreleased.