-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
_debug_agent: use readableObjectMode
option for client stream
#270
Conversation
R=@indutny? The change LGTM but maybe there's a reason why it initializes like that. |
LGTM, if it works! |
Does exactly same thing: https://github.com/iojs/io.js/blob/v1.x/lib/_stream_readable.js#L43 |
@vkurchatkin Can you amend the commit log? The first line should be <= 50 characters. |
@vkurchatkin Can you also add a commit message describing the motivation for this change. Because I can't reconstruct why this is needed/nice. |
Use public `readableObjectMode` option to construct `Transform` instead of accessing private `_readableState.objectMode`.
8ae8a9c
to
566b3e4
Compare
@bnoordhuis done |
There other places (a lot, actually) where |
@vkurchatkin Are you volunteering to clean them up? :-) |
I'm afraid it's not as simple as cleaning up. The first thing to do is to categorize all the cases and create issues. In general I see these possible resolutions (sorted by preference):
Can start by doing this) |
Internal streams reaching into / through That said, I'm not opposed to going through that process over the course of commits cleaning up smaller transgressions one-by-one. Unless anyone has strong feelings about it, I'd like to merge this PR as-is. We can track progress on the other places in a separate issue -- @vkurchatkin, would you like to open that up? |
@chrisdickinson what about this PR? |
lgtm too |
Merging as we speak. |
Use public `readableObjectMode` option to construct `Transform` instead of accessing private `_readableState.objectMode`. Partially addresses #445. PR-URL: #270 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Merged in 9e62ae4. Thanks! The first shots have been fired in the struggle to cleanup stream usage 💥! |
No description provided.