stream: simplify checks for highWaterMark #19893
Labels
good first issue
Issues that are suitable for first-time contributors.
stream
Issues and PRs related to the stream subsystem.
Comments
mcollina
added
stream
|
So line lib/_streams_readable.js#314 and line lib/_streams_readable.js#437 needs to be replaced. Removing |
|
@mcollina If no one is already working on this, I would like to volunteer. First-time node contributor. |
|
@sagirk I have started working on the issue |
|
i also have started to work on it |
kodemill
added a commit
to kodemill/node
that referenced
this issue
May 28, 2018
Inline the needMoreData function since it has only one call place. Update the related comment. Add a test for the edge case where HWM=0 and state.length=0. Add a test for ReadableStream.read(n) method's edge case where n, HWM and state.length are all zero. This proves that there is no easy way to simplify the check at https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L440 Fixes: nodejs#19893 Refs: nodejs#19896
|
@mcollina @vishal7201 I made some progress on this, since it looked like it was orphaned. |
MylesBorins
pushed a commit
that referenced
this issue
Jun 6, 2018
Inline the needMoreData function since it has only one call place. Update the related comment. Add a test for the edge case where HWM=0 and state.length=0. Add a test for ReadableStream.read(n) method's edge case where n, HWM and state.length are all zero. This proves that there is no easy way to simplify the check at https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L440 Fixes: #19893 Refs: #19896 PR-URL: #21009 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In
lib/_streams_readable.js, we have a block of code like this:Because
state.highWaterMarkcould be< 0ornullorundefinedor really any object in Node < 10. However, we landed #18098 and so now we can simplify all those checks to bestate.length <= state.highWaterMark.The text was updated successfully, but these errors were encountered: