fix: avoid using deprecated Buffer constructor

[ChALkeR]

Buffer.alloc() is supported on Node.js >= 4.5.0, and allocates a zero-filled
buffer on all supported versions.

This solves two issues:

• Buffer() constructor (aka 'new Buffer()') is deprecated, so avoid using it
• On 4.x and 6.x, Buffer(number) is not zero-filled, so this code was
  allocating an uninitialized Buffer when file length was less than 150, so
  the behaviour of readShebang() was actually undefined in that case (as in
  could have returned anything, depending on the uninitialized memory chunk).

Refs: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor

Note: I have not run tests on this, but expect no issues. CI should verify that.

[satazor]

The build is failing due to an unrelated error. I will try to fix it.

[satazor]

I've fixed the master branch, lets wait for it to pass.

[satazor]

@ChALkeR Some node versions that we are targeting in appveyor are failing. Should we do a fallback to new Buffer + zero filling?

[ChALkeR]

@satazor What is the targeted Node.js version range that you intend to support? I can adjust the PR accordingly and add fallbacks, but I don't recommend to.

Not sure why supporting Node.js 5.x is desired — it wasn't an LTS release and reached EOL in June 2016.
Ref: https://github.com/nodejs/Release#release-schedule

See also https://nodesource.com/node-by-numbers — the numbers for 5.x are below even those of 0.12.

My recommendation would be to just drop ≤4.5.0 and 5.x support and use Buffer.alloc nowdays, but I can add fallback to new Buffer() if you want me to. Yes, it will use .fill(0) to fix the bug, but it will need Buffer.alloc detection to avoid calling deprecated API on newer Node.js versions.

[satazor]

I agree with your suggestion to just drop support for EOL node versions. Unfortunately, that will be a breaking change. I think that we can get around it for now and simply remove the fallback on the next major release that will contain some more breaking changes.

Could you please update the PR to do the fallback?

[satazor]

You may look at the array of supported versions here: https://github.com/moxystudio/node-cross-spawn/blob/master/.travis.yml

[satazor]

Bump.

[ChALkeR]

@satazor Yup, will do it in today or tomorrow.

[codecov]

Codecov Report

Merging #94 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   98.86%   98.88%   +0.02%     
==========================================
  Files           7        7              
  Lines         176      180       +4     
  Branches       38       39       +1     
==========================================
+ Hits          174      178       +4     
  Misses          2        2

Impacted Files	Coverage Δ
lib/util/readShebang.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 213aa43...433f947. Read the comment docs.

[ChALkeR]

@satazor Done. There are three logically separate commits.
To drop outdated Node.js versions support, revert the last one.
I have not tested that, but should be fine if I didn't make a mistype ;-). Let CI test things.

[ChALkeR]

Travis failures seem unrelated?

[satazor]

Yes they are unrelated. Somethings wrong with the babel preset. Will look into it soon.

[ChALkeR]

Tracking: nodejs/node#19079

[realityking]

Since cross-spawn already requires Node.js 4.8 or newer, how about making a major release and requiring >=4.5.0 <5.0.0 || >=5.10.0. That way the workaround for old Node versions could be removed.

[satazor]

@realityking In the next major release, I will require node >= 6.

Regarding the failing travis build, it will hopefully pass now.