-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
update deps, modernise usage, use readable-stream #49
Conversation
this is all semver-patch safe for anything Node 6 and above, semver-minor could be a good idea to send a signal about amount of change if you like |
should we be using safe-buffer (which is what we do for a bunch of other crytpo-browserify libs so we probably wouldn't be adding it to the deps) |
No, safe-buffer is redundant these days. I think we were even shipping the new Buffer APIs back in a later release of version 4. All supported release lines now support the new APIs and as long as you're using them directly then there's nothing safe-buffer can add and it ends up being both a noop and dead-weight in your dependency tree: var buffer = require('buffer')
var Buffer = buffer.Buffer
...
if (Buffer.from && Buffer.alloc && Buffer.allocUnsafe && Buffer.allocUnsafeSlow) {
module.exports = buffer
} else {
// Copy properties from require('buffer')
copyProps(buffer, exports)
exports.Buffer = SafeBuffer
} In other words, if you're still using safe-buffer, it's time to remove it. |
Related with #47 .... |
so we need to figure out how we are going to support both browserify and webpack when wepback starts forcing you to requiring 'buffer' while browserify doens't allow you to require buffer (you need to require('buffer/')) |
Regarding bn.js upgrade issues, there probably solution (indutny/bn.js#239 (comment)) which will allow use v4.x and v5.x at one time, but while bn.js is not updated in libs each bundle will have two bn.js versions =( |
ah fuck I missed that this upgraded that, ok we should revert that |
Or we should force bn.js v5.x everywhere :) |
Unfortunately this was a breaking change because the node version v4 must support are >= 4, and readable-stream v3 requires node 6. @rvagg what would it break to downgrade readable stream? |
No, I don't think it would break. Is this about cutting a release? 3 years after merge? Caring about a version of Node.js that's been unsupported for more than 5 years? Maybe none of this matters and do whatever feels most comfy to you. |
Thankfully everything actually works on node 4, so im instead going to pursue a readable-stream v3 patch that lowers the support level. node’s support of a version is irrelevant to semver; this major line supports what it supports and can’t drop anything from that until v5. |
some modernisation:
new Buffer()
toBuffer.alloc()
andBuffer.from()
require('buffer').Buffer
to make it safe for Webpack 5 et. al. that are dropping node.js buildins.