-
Notifications
You must be signed in to change notification settings - Fork 64
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
Use Buffer.(from|alloc) instead of deprecated Buffer API #103
Conversation
This also includes a dependnecy on a polyfill targeting older Node.js versions where Buffer.alloc() and Buffer.from() API is not implemented (Node.js < 4.5.0 and some 5.x versions). Fixes: indutny#102 Ref: nodejs/node#19079 Ref: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor Ref: https://nodejs.org/api/buffer.html#buffer_class_buffer Ref: https://github.com/ChALkeR/safer-buffer/blob/master/Porting-Buffer.md
/cc @indutny |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@dcousens would you care to take a look too?
@ChALkeR I just read https://github.com/ChALkeR/safer-buffer#why-not-safe-buffer, sane enough.
Is the discussion for why That sounds against my impression of what the point of that module was for, which was to provide the safe implementation (warnings et al) in older versions... If it didn't provide that, imho it should, and we should avoid the package double-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only question remains why safer-buffer
@dcousens That behavior is both documented all over Readme, verified in the tests, and even the implementation itself just returns unchanged Node.js There would be no big difference between having it a semver-major version of |
@feross, thoughts? Should we bump the ecosystem to |
@indutny I'm happy to +1 this now as is, but, I am curious to the above question. Merge when ready. |
@dcousens I don't think that bumping that part of the ecosystem is worth the churn, there are many yet unpatched modules, we better look at those first. The anticipated path forward would be to drop old Node.js versions support and any of that kind of polyfills in the next semver-major versions. |
/ping @indutny |
@dcousens Btw, |
The point of The remaining users still using Node 10 will be LTS later this year at which point this deprecation warning will be everywhere, filling up everyone's issue trackers. |
@feross, fwiw |
@dcousens That's interesting, haha. Either way, I don't think we need to change |
@fanatid @calvinmetcalf shall we |
Shouldn't that be https://www.npmjs.com/package/buffer ? That's a completely different package. |
@ChALkeR you don't want to include
It isn't clear to me if (or when) we can go straight to |
@dcousens Something looks not correct there, it shouldn't be that way. I.e. it shouldn't be needed for users to include any of the It's bad, if that's the case. So, some questions:
|
@ChALkeR indeed, writing the above made me re-think that. I think this is a conflation of two issues that had merged (in my head) due to their timing when we implemented them.
I think you are correct in that, if we can drop the polyfill, we can replace |
my rule of thumb is that browserify code tends to end up being used in unexpected environments so while I can't think of any scenarios where this could run where buffer.alloc isn't available due to an old buffer, I wouldn't rule it out, plus a lot of other libraries related to this one are also requiring safe-buffer so there is probably 0 marginal cost of package size in having safe-buffer |
Needs rebase |
Any change on this issue in the last year ? I would love seeing this merged. |
Will rebase this today or tomorrow. |
@ChALkeR are you planning remove |
Rebased and landed in e186699. Thank you! I have absolutely no excuse for not landing it earlier. Hopefully you don't hold a grudge against me for this. |
@indutny By now I think it's ok to just drop Node.js < 4.5 support, together with the polyfill, and make it a major version (as noted in the original post). Thoughts? |
This also includes a dependnecy on a polyfill targeting older Node.js versions where Buffer.alloc() and Buffer.from() API is not implemented (Node.js < 4.5.0 and some 5.x versions).
Fixes: #102
Ref: nodejs/node#19079
Ref: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor
Ref: https://nodejs.org/api/buffer.html#buffer_class_buffer
Ref: https://github.com/ChALkeR/safer-buffer/blob/master/Porting-Buffer.md
Also, I would suggest to use this the polyfill on 5.x branch (so that existing users would receive the fix), but also to release a new 6.0.0 version, dropping
safer-buffer
dependency and outdated Node.js <4.5.0 (or even <6.0.0) support.Also, could this be backported to the 4.x branch? That's what most users are observing and where they can see the warning from. Should land cleanly on that.