Avoid using deprecated Buffer API on newer Node.js #67
Conversation
This avoids using Buffer constructor API on newer Node.js versions. To achieve that, Buffer.from presence is checked, with validation that it's not the same method as Uint8Array.from. Also an additional type-guard is added in the fallback code path to ensure that typed numbers are never accidently fed into `new Buffer` input. Refs: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor
|
Ping? |
| signatureBuffer = Buffer.from(signatureBase64, 'base64'); | ||
| } else { | ||
| // Node.js <4.5.0 || >=5.0.0 <5.10.0 | ||
| if (typeof signatureBase64 === 'number') { |
There was a problem hiding this comment.
If we're going to guard against invalid input, wouldn't it make more sense move this check higher? Perhaps it should also be formulated as an assert since that's what's being done elsewhere in this code e.g.
// ...
var signatureBase64 = parsedSignature.params.signature;
assert.string(signatureBase64, 'signatureBase64');
var signatureBuffer;
// ...
There was a problem hiding this comment.
The assertion is made by Buffer.from internally. Although I agree assert.string can be used for the fallback rather than the type checking.
A
TypeErrorwill be thrown ifstringis not a string.
Source: https://nodejs.org/docs/latest-v10.x/api/buffer.html#buffer_class_method_buffer_from_string_encoding
There was a problem hiding this comment.
OK, so, IIUC, the rationale for pushing the check down into the fall-back code is that it makes it more obvious that it can be removed when the fall-back code is removed. I guess that makes sense. It just seemed a little odd to me that there are ~10 lines of code just to use Buffer.from if it's available...maybe I'm naïve, but it seems like it ought to be a one-liner.
This avoids using Buffer constructor API on newer Node.js versions.
To achieve that,
Buffer.frompresence is checked, with validation that it's not the same method as Uint8Array.from.Also an additional type-guard is added in the fallback code path to ensure that typed numbers are never accidently fed into
new Bufferinput.Given this module popularity and the fact that old Buffer constructor API was used in a single place, I decided to just feature-detect it in-place instead of pulling in polyfill deps.
Another way would be to just use
Buffer.fromand dovar Buffer = require('safe-buffer');Refs:
https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor
Tracking:
nodejs/node#19079