-
Notifications
You must be signed in to change notification settings - Fork 54
Replace deprecated Buffer constructor #99
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
Conversation
lib/api.js
Outdated
| try { | ||
| buffer = Buffer.from(key); | ||
| } catch (e) { | ||
| if (e.message === 'this is not a typed array.') { |
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.
Why need check this?
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.
Buffer.from suppose to officially replace new Buffer in v5.x, which suppose to support any parameter that is supported by Buffer(), however the oldest version that we still support (v4.4.6.) also implemented Buffer.from that which would throw this is not a typed array. if key isn't an array.
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.
I think this should check the existence method instead of catching and checking the error message when method is not exist in older version of node.
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.
By the way, there was a backport of Buffer API to v4.x in v4.5.0. So, the Buffer.from() in versions prior is not the same thing. Please checking the existence of Buffer.allocUnsafe() instead.
// backward compatilibity for 4.x
if (typeof Buffer.allocUnsafe === "function"){
buffer = Buffer.from(key);
} else {
buffer = new Buffer(key);
}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.
@aquartier Checking for existence of Buffer.from isn't enough, because it exist in v4 as well.
allocUnsafe is a good idea. Thanks
Bufferconstructor has been deprecated because of security reasons and has been replaced withBuffer.fromhttps://nodesource.com/blog/understanding-the-buffer-deprecation-in-node-js-10/