Avoid using deprecated Buffer API on newer Node.js

[ChALkeR]

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.

The !number type-guard for older Node.js versions is excluded and replaced with a comment, as binary is always a string there and this looks to be potentially hot code path.

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.from and do var Buffer = require('safe-buffer');

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

[dougwilson]

Awesome, thanks! Looks like the tests are failing with the change, and I don't mind adding in safe-buffer module like everything else; ultimately Buffer may be a stopgap if people end up getting more encoding supported added in the module anyway. Plus standard format will actually just complain about the usage anyway, even if it's surrounded by the check.

I'm curious: is Buffer.from !== Uint8Array.from actually necessary to check in addition to just checking that Buffer.from is present?

[ChALkeR]

@dougwilson, Yeah, the tests were failing because I made a stupid mistype in this PR :-/. Fixed in 7c1855d. Sorry for that.

Buffer.from !== Uint8Array.from is necessary as 4.x < 4.5.0 has Buffer.from which is essentially Uint8Array.from which doesn't work that way.

8eaecc5 doesn't fix this issue as it doesn't load safe-buffer.

Also, I have concerns with safe-buffer usage, namely — throwing it in will effectlively break the lint rule that should avoid Buffer constructor usage. I documented that at https://github.com/chalker/safer-buffer#why-not-safe-buffer. It should probably be fine in this module though as Buffer constructor is used in a single place.

[dougwilson]

Ah, thanks for the explanation! And thanks for your well-written things. I agree with your safer-buffer module in that is polyfills + removes the deprecated behavior. I didn't know of it (or maybe it wasn't there yet) when I began moving all the modules over to safe-buffer, but I think I may go back and sweep all the modules to safer-buffer for your reasons above (though at least release this with safe-buffer so folks can get an Express.js version that has a single buffer polyfill in the tree).

Re the comment: yea, that was my bad; I missed that in the git add -p, but as you saw got it fixed up 👍

As a side-note, buffer -> safe-buffer -> safer-buffer progression reminds me of the Perl ecosystem. Someone needs to make a safest-buffer now ;)

[ChALkeR]

@dougwilson My suggestion would still be to use neither of those and use the approach from 7c1855d instead to remove an extra dependency whatsoever for 0.5.3 (imo the diff of that size doesn't worth a dependency), and to revert this in favor of just Buffer.from in 0.6.0, dropping support for outdated Node.js versions.

[ChALkeR]

That said, the current approach in d542fa5 looks fine to me. Just not something that I would have done =).

[zoe-mcelderry]

@dougwilson Will there be a new release of this package soon that includes this fix please?

[dougwilson]

Hi @zoe-mcelderry I apologize; I didn't realize I never released this after merging it. I will make it a priority for when I have time tonight.

[zoe-mcelderry]

Many thanks!

[dougwilson]

Hi @zoe-mcelderry I just released only this change in version 0.5.3. Again, sorry it got forgotten. I'll take a look at the outstanding PRs / issues later today, but just wanted to get this release out for you first 👍

[zoe-mcelderry]

Thank you :)