stream: eos use writableFinished when available #28749
Conversation
| @@ -35,7 +35,8 @@ function eos(stream, opts, callback) { | |||
| if (!stream.writable) onfinish(); | |||
| }; | |||
|
|
|||
| var writableEnded = stream._writableState && stream._writableState.finished; | |||
| var writableEnded = stream.writableFinished || | |||
There was a problem hiding this comment.
The whole point of these new getters is to avoid using {_readable,_writable}State but in this case if the getter exists and returns false we use _writableState twice (first time in the getter itself and second time in the RHS of the "or" condition) so I'm not a fan of this change.
There was a problem hiding this comment.
don't you think V8 is able to figure that out?
There was a problem hiding this comment.
Is there a desire to use writableFinished instead of _writableState in files such as this one, that are internal to streams, or should we only be doing that outside of the streams module itself (for example, in lib/http.js).
I thought the answer was "Only in files outside of the streams module itself."
The answer is relevant to determining how much work remains to be done to close what is literally the oldest open issue we have: #445.
There was a problem hiding this comment.
Oh, wait, I see there's the "Improves compat with streamlike objects." comment, so the motivation isn't expanding usage of writableFinished for its own sake.
There was a problem hiding this comment.
Good point, I also missed that. It makes more sense now, approving.
Trott
dismissed
their stale review
thinking about this some more, probably could use a test?
|
Would it be not-too-onerous to add a test that would fail on master but pass with this change? |
|
/ping @ronag Are you interested in adding a test to this? |
|
This is covered by #28748 |
Use writableFinished when available. Improves compat with streamlike objects.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes