Emit close event once we reach the end of the stream #24
Conversation
src/Stream/ReadableStreamTrait.php
Outdated
| @@ -102,6 +102,9 @@ protected function performRead($chunkSize) | |||
| $this->emit('end', [ | |||
| $this, | |||
| ]); | |||
| $this->emit('close', [ | |||
| $this, | |||
| ]); | |||
There was a problem hiding this comment.
Makes sense. Maybe call close() instead here? Could also use a test or two 👍
There was a problem hiding this comment.
Replacing both lines with close and calling close instead. Will also add tests to be sure everything works as intended 👍
There was a problem hiding this comment.
Solved the issue, working on tests now. (It required some other related code change as well.)
…djusted a test to test the expected workflow. Follow up for reactphp#24 (comment) from @clue
src/Stream/ReadableStreamTrait.php
Outdated
| $this->emit('end', [ | ||
| $this, | ||
| ]); | ||
| $this->close(); |
There was a problem hiding this comment.
I'm not sure I follow exactly what' supposed to happen here, but it's my understanding that this should first emit an end event immediately followed by a close? (https://github.com/reactphp/stream#end-event)
Do we have a test for this?
There was a problem hiding this comment.
Good point, end shouldn't be emitted from the close call. But from when we reach EOF. Adjusted code and tests to ensure the behavior is as described on https://github.com/reactphp/stream#end-event
There was a problem hiding this comment.
Thank you for looking into this, changes LGTM! 👍
For the reference: The additional $this arguments have been removed via reactphp/stream#69 a while back. This component still targets legacy stream v0.4, so I guess we may want to address this eventually.
|
Cool, I'll squash and merge soon 👍 |
40c95fb to
2b9907e
Compare
No description provided.