-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
stream: complete migration to internal/errors #16589
Conversation
@nodejs/tsc this needs expedite, as we need it for the 9 release. I'm sorry, but I caught these just now. |
I think @jasnell wanted to build the binaries ~ now? I’m okay with expediting but maybe we have the time for a CITGM run as well? |
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.
One bit but generally lgtm and +1 on fast tracking
lib/_stream_readable.js
Outdated
@@ -284,7 +284,7 @@ function chunkInvalid(state, chunk) { | |||
typeof chunk !== 'string' && | |||
chunk !== undefined && | |||
!state.objectMode) { | |||
er = new TypeError('Invalid non-string/buffer chunk'); | |||
er = new errors.TypeError('ERR_STREAM_INVALID_CHUNK'); |
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 probably just use ERR_INVALID_ARG_TYPE here
lib/_stream_writable.js
Outdated
} else if (typeof chunk !== 'string' && | ||
chunk !== undefined && | ||
!state.objectMode) { | ||
er = new TypeError('Invalid non-string/buffer chunk'); | ||
er = new errors.TypeError('ERR_STREAM_INVALID_CHUNK'); |
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.
And here
<a id="ERR_STREAM_CANNOT_PIPE"></a> | ||
### ERR_STREAM_CANNOT_PIPE | ||
|
||
Used when an attempt is made to call [`stream.pipe()`][] on a |
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.
Missing link definitions, so are Writable
and stream.write()
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.
added
Complete the migration to the new error system of _stream_readable and _stream_writable. Adds the corresponding documentation.
d464d69
to
2651f29
Compare
I'll be doing one final pull from master this afternoon and will be kicking off the release build later on today. |
Landed as 6e86a66 |
Complete the migration to the new error system of _stream_readable and _stream_writable. Adds the corresponding documentation. PR-URL: #16589 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Complete the migration to the new error system of _stream_readable and _stream_writable. Adds the corresponding documentation. PR-URL: nodejs/node#16589 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Complete the migration to the new error system of _stream_readable and _stream_writable. Adds the corresponding documentation. PR-URL: nodejs/node#16589 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Complete the migration to the new error system of _stream_readable and _stream_writable. Adds the corresponding documentation. PR-URL: nodejs/node#16589 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
stream