Use Stream 0.5 events semantic #87
Conversation
clue
left a comment
There was a problem hiding this comment.
Nice, thanks for the high quality changes, this is really much appreciated! 🎉
Most of the changes LGTM, but afaict the Request::end() does not follow stream semantics yet. Does it make sense to address this as part of this PR? 👍
| "react/stream": "0.4.*", | ||
| "react/dns": "^0.4", | ||
| "react/event-loop": "^0.4", | ||
| "react/stream": "^0.5|^0.6", |
There was a problem hiding this comment.
We can probably also keep BC with 0.4 here.
| } | ||
|
|
||
| $this->close($error); | ||
| public function handleClose() |
There was a problem hiding this comment.
Should probably be marked @internal (see also others?) or we may even use close() as the event handler here.
| { | ||
| $this->closeError(new \RuntimeException( | ||
| "Connection closed before receiving response" | ||
| )); |
There was a problem hiding this comment.
👎 This should probably stay in the end handler. The end event will be emitted when the connection has ended, the close event will also be emitted if you explicitly call close() in which case this would not be considered an error.
Following #83 (review)
Changes in Request, Response and tests