-
Notifications
You must be signed in to change notification settings - Fork 230
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
readable-stream@3 #344
readable-stream@3 #344
Conversation
* add airtap for continuous browser tests * ad-hoc fixes for browser tests * comment out failing browser tests * restore safari * Updated. * more airtap quirks * remove --no-coverage for airtap * better sauce credentials for travis * Maybe better env variables for airtap * removed sauce credentials from .travis.yml * added back all browsers * added back loopback host * Updated README links for sauce and travis * Suport for IE11
I've just released readable-stream@3.0.0-rc.1. Install it with |
util.inspect.custom = 'custom'; | ||
} | ||
|
||
// TODO: add replacements instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be replaced before release?
require('./browser/test-stream2-readable-from-list')(t); | ||
require('./browser/test-stream2-transform')(t); | ||
// require('./browser/test-stream2-readable-from-list')(t); | ||
// require('./browser/test-stream2-transform')(t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code?
return 'The ' + name + ' method is not implemented'; | ||
}); | ||
createErrorType('ERR_STREAM_PREMATURE_CLOSE', 'premature close'); | ||
createErrorType('ERR_STREAM_DESTROYED', 'the stream was destroyed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used capital letters in Node.js for the beginning of the message.
In core ERR_STREAM_DESTROYED
has the message: 'Cannot call %s after a stream was destroyed'
createErrorType('ERR_UNKNOWN_ENCODING', function (arg) { | ||
return 'Unknown encoding: ' + arg; | ||
}, TypeError); | ||
createErrorType('ERR_STREAM_UNSHIFT_AFTER_END_EVENT', 'stream.unshift() after end event'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages slightly diverge in general from core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node.js test suite passes without changing anything regarding error codes. Would you be able to send a PR against this branch that updates the error messages to the one of Node.js core? All of those were ported by hand :/.
On node 8.11.1, readable-stream crashes the process with Do we really need that? Why would it not be included? |
working in my small test now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an enormous diff, but most of that seems to be from copying code from node core.
Given the amount of tests, and the fact that they all pass that seems to be the best measure of whether or not this works. The fact that this is a major release also somewhat lowers the stakes of accidentally breaking things in userland.
LGTM!
we probably want a changelog or a notice in the readme about what breaking changes are in v3 to make upgrading easier, otherwise LGTM |
@calvinmetcalf done. |
As titled, this is a port of Node.js 10.5.0.
It includes all the semver-major changes of both Node 9 and 10.
Fixes #283
Fixes #284
Fixes #212
Fixes #297
Fixes #346
Fixes #339
Fixed #335