fixes shebang execution on Node 5.8.x

[bcoe]

working from:

#12

fixes shebang handling on Node 5.8.x

looks to be almost identical to:

https://github.com/tapjs/spawn-wrap/pull/13/files

so I'll let @isaacs decide what we land.

[addaleax]

As mentiond in the nyc issue, this shebang replacement only applies to the npm executable – I think it would be a good idea to mention that here, otherwise the comment might be a little confusing?

Also, this “special” shebang approach itself is quite old, it’s the combination of it with the npm version shipped with v5.8.0 that creates the issue.

[bcoe]

updating.

[addaleax]

I think some parts of this are orthogonal to #13: You only check for the “strange” shebang installed by Node.js executables (which #13 doesn’t), but there’s no reason not to catch something like #!/usr/local/bin/node

Anyway, both skipping any tests on Windows and using which + head sound like good ideas.

[bcoe]

@isaacs @addaleax having slept on this, I think @addaleax's original approach of only applying this logic to the npm bin is smart ... this is definitely an edge-case, but it impacts a large number of users since nyc is depended on by 1000ish projects at this point.

tldr; I think this is a great hack to have, but we should limit applying it to as small a scope as possible. I like that with this approach:

• we only need to read from a file once we determine it is the npm bin.
• we only apply the hack to versions of Node >1.x

[addaleax]

Hm, yeah… as @isaacs pointed out in #12 (comment), the question of the scope of this fix is not easy. I’d tentatively agree with the behaviour that #13 seeks to achieve, namely that any script with a shebang invoking the node binary using a full path should also be wrapped when using spawn-wrap.

[addaleax]

Also, just btw, if you run make install from a Node.js core source tree you end up with a full-path shebang like #!/usr/local/bin/node. That one would be caught by #13, but not by this PR (as it is), if I understand correctly?

[addaleax]

Um, I just saw these files from the npm-cli repo: bin/npm bin/npm.cmd. I don’t really know what’s up with them, but I imagine there are cases where these are used when npm is invoked from the command line?
If so, I’m tempted to change my opinion again – possibly even back to just checking for “npm” on the command line, no shebang magic stuff – because both of them carry out the ``dirname "$0"/node-approach in different, even less “detectable” ways.

[isaacs]

I think I'd like to land #12 and #13. (That is, handle absolute shebang but also special case for "npm".)

Reading and parsing the bash exec junk seems like a step too far.

I also plan to make the case for removing this hack from the node installer. It's overreaching. If someone out there really wants /some/weird/path/npm to be executed by /some/weird/path/node, then can invoke it explicitly themselves. Otherwise, the npm on the PATH should rely on the node on the PATH using /usr/bin/env node, as the unix gods intended.

[isaacs]

@addaleax those files are only installed on Windows I believe.

[addaleax]

@isaacs Sounds good to me. Do you want me to clean up #12 to use which, or would you prefer to do that yourself?