Skip to content

update deps, modernise usage, use readable-stream#49

Merged
calvinmetcalf merged 5 commits into
browserify:masterfrom
rvagg:rvagg/readable-stream
May 5, 2020
Merged

update deps, modernise usage, use readable-stream#49
calvinmetcalf merged 5 commits into
browserify:masterfrom
rvagg:rvagg/readable-stream

Conversation

@rvagg

@rvagg rvagg commented May 4, 2020

Copy link
Copy Markdown
Contributor

some modernisation:

  • use readable-stream rather than stream to prevent breakage and fix to a specific version (stream_browserify is currently being a pain because of upstream breakage)
  • update dependencies, fix all security warnings
  • update standard and fix all linting errors
  • switch from new Buffer() to Buffer.alloc() and Buffer.from()
  • skip tests for signatures where the algorithm isn't supported by Node.js (main algos that use DSA and SHA1 which have been removed from the default OpenSSL bundle now used)
  • update travis descriptor to run on latest versions of Node.js
  • (edit: updated) added explicit require('buffer').Buffer to make it safe for Webpack 5 et. al. that are dropping node.js buildins.

@rvagg

rvagg commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

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

@calvinmetcalf

Copy link
Copy Markdown
Contributor

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)

@rvagg

rvagg commented May 5, 2020

Copy link
Copy Markdown
Contributor Author

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:

https://github.com/feross/safe-buffer/blob/7d544741d5a360613abb0d47926e107722fae723/index.js#L11-L13

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.

@calvinmetcalf calvinmetcalf merged commit 92b2b83 into browserify:master May 5, 2020
@fanatid

fanatid commented May 20, 2020

Copy link
Copy Markdown
Contributor

Related with #47 ....

@calvinmetcalf

Copy link
Copy Markdown
Contributor

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/'))

@fanatid

fanatid commented May 20, 2020

Copy link
Copy Markdown
Contributor

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 =(

@calvinmetcalf

Copy link
Copy Markdown
Contributor

ah fuck I missed that this upgraded that, ok we should revert that

@fanatid

fanatid commented May 20, 2020

Copy link
Copy Markdown
Contributor

Or we should force bn.js v5.x everywhere :)
I actually think that if bn.js v4.x patch will be successful revert here will not be required.

@rvagg rvagg deleted the rvagg/readable-stream branch May 26, 2020 04:55
@ljharb

ljharb commented Nov 27, 2023

Copy link
Copy Markdown
Member

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?

@rvagg

rvagg commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

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.

@ljharb

ljharb commented Nov 28, 2023

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants