-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: avoid using deprecated Buffer constructor #94
Conversation
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
The build is failing due to an unrelated error. I will try to fix it. |
I've fixed the master branch, lets wait for it to pass. |
@ChALkeR Some node versions that we are targeting in appveyor are failing. Should we do a fallback to |
@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. 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 |
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? |
You may look at the array of supported versions here: https://github.com/moxystudio/node-cross-spawn/blob/master/.travis.yml |
Bump. |
@satazor Yup, will do it in today or tomorrow. |
'150' was used two times, it deserves a name. It's the size of the pre-read buffer.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
To be reverted when dropping outdated Node.js versions support.
@satazor Done. There are three logically separate commits. |
Travis failures seem unrelated? |
Yes they are unrelated. |
Tracking: nodejs/node#19079 |
Since |
@realityking In the next major release, I will require node >= 6. Regarding the failing travis build, it will hopefully pass now. |
Buffer.alloc() is supported on Node.js >= 4.5.0, and allocates a zero-filled
buffer on all supported versions.
This solves two issues:
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.