Closed ChALkeR closed 4 years ago
/cc @indutny
@ChALkeR I just read https://github.com/ChALkeR/safer-buffer#why-not-safe-buffer, sane enough.
After some discussion, it was decided to move my approach into a separate package, and this is that separate package
Is the discussion for why safe-buffer
doesn't throw the deprecation warnings on unsafe behaviour public?
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.
@dcousens safe-buffer
design decision was to keep Buffer()
constructor support for whatever reason. That was done long ago, and I don't have a good idea why they did that, probably some historic reasons (like the Buffer constructor itself).
That behavior is both documented all over Readme, verified in the tests, and even the implementation itself just returns unchanged Node.js Buffer
on Node.js versions that have the new API. That is not possible to fix with several lines of code — changing all that is going to be a huge backwards-incompatibility rewrite of safe-buffer
(in fact, that's what safer-buffer
is — a complete rewrite). Afaik, the authors of safe-buffer
are not going to change that — they are going to rely on Node.js to deprecate Buffer()
constructor instead for messaging.
There would be no big difference between having it a semver-major version of safe-buffer
and in another module named safer-buffer
, btw — even in the first case those would not have been dedupeable.
@feross, thoughts? Should we bump the ecosystem to safer-buffer
?
@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, buffer-from
and buffer-alloc
ponyfills are also a good alternative, combined with a linter. Perhaps I need to add them to the doc. They add some depenency churn, though.
The point of safe-buffer
is to give you the ability to use the new, safer Buffer APIs in old versions of Node (<4.5.0). But since Node 4 is now EOL, going forward it makes sense to drop support for Node 4 which removes the need for any kind of polyfill.
The remaining users still using safe-buffer
in their projects will start to get runtime deprecation warnings from Node 10 if they're still using new Buffer()
instead of the new APIs. Rather than make a new breaking version of safe-buffer
to print this warning ourselves, we'll just let the Node 10 users see it, report it on issue trackers, and eventually this will be fixed in any module that is actively maintained.
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 safe-buffer
has also been used as a way to "fix" environments where Buffer
may not be automatically imported (cough, react-native
)
@dcousens That's interesting, haha. Either way, I don't think we need to change safe-buffer
behavior here.
@fanatid @calvinmetcalf shall we s/safe-buffer/safer-buffer
? Oh browserify woes.
This is a mess.
@dcousens
safe-buffer
has also been used as a way to "fix" environments whereBuffer
may not be automatically imported
Shouldn't that be https://www.npmjs.com/package/buffer ? That's a completely different package.
It could be used without safe-buffer
as it already provides the same API by itself.
Or am I missing something?
@ChALkeR you don't want to include buffer
as a npm
dependency on Node, as you wouldn't want a JS polyfill.
safe-buffer
works in that you only get the polyfill in a browserify environment.
safer-buffer
would provide the same behaviour AFAIK.
It isn't clear to me if (or when) we can go straight to buffer
or if we need to use safer-buffer
.
@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 safe-buffer
/safer-buffer
to get Buffer working in browserify
.
It's bad, if that's the case.
So, some questions:
safe-buffer
/safer-buffer
better in that aspect than just writing Buffer = require('buffer')
but not adding buffer
to the deps? Because that's exactly what those two packages do.Buffer = require('buffer')
and just using global Buffer
directly? I verified that browserify
works just fine on just a single Buffer.alloc(10)
line and packages in the buffer dependency.@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.
safe-buffer
poly-fill for old Node versionsreact-native
needs an explicit require('buffer')
I think you are correct in that, if we can drop the polyfill, we can replace safe-buffer
with require('buffer')
solely and maintain no package dependency.
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 safe-buffer
or still leave it but rebase for removing conflicts?
Rebased and landed in e186699e18fd45158bfa25225c2ead5804a42d9a. 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: https://github.com/indutny/asn1.js/issues/102
Ref: https://github.com/nodejs/node/issues/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.