First draft of RFC for websockets API. #113
Conversation
|
Can someone explain to me how to link the rendered document without it being tied to a specific blob? |
|
ping |
|
I would like to assist. The draft of the API looks complete enough for an experimental implementation. What would be the best next steps for this? |
|
I just saw this. I would suggest using an API based on channels/streams as that's consistent with other websocket crates and, imo, is more "rust-y". See this implementation of such an API in |
|
The |
|
Seems that way. I somehow missed the figured part. This API looks good. |
| /// Lose access to the websocket but keep the callbacks in case any events are recieved. | ||
| /// | ||
| /// It's best not to use this function in production, as the callbacks and possibly the | ||
| /// websocket itself will leak. TODO should this method exist at all? | ||
| pub fn forget(mut self) { .. } |
There was a problem hiding this comment.
Imo, this shouldn't exist. We should store the callbacks and drop them when the WebSocket instance is dropped.
| on_open: impl FnOnce() + 'static, | ||
| mut on_error: impl FnMut() + 'static, | ||
| on_close: impl FnOnce(CloseEvent) + 'static, | ||
| mut on_message: impl FnMut(Message) + 'static, |
There was a problem hiding this comment.
Should this be using a builder? I'm not sure if forcing the user to define empty callbacks is a good idea.
| pub fn forget(mut self) { .. } | ||
|
|
||
| /// Start the closing handshake. | ||
| pub fn close(&self) { .. } |
There was a problem hiding this comment.
close should take ownership so the instance can't be used for being closed.
| pub fn close(&self) { .. } | |
| pub fn close(self) { .. } |
| /// The code must be *1000* or between *3000* and *4999* inclusive, and the reason string, if | ||
| /// present, must be less than or equal to 123 bytes in length (when utf8 encoded). If either | ||
| /// of these conditions are violated, the function will error without closing the connection. | ||
| pub fn close_with_reason(&self, code: u16, reason: Option<&str>) -> Result<(), CloseError> { .. } |
There was a problem hiding this comment.
close_with_reason, like close, should take ownership so the instance can't be used for being closed.
| pub fn close_with_reason(&self, code: u16, reason: Option<&str>) -> Result<(), CloseError> { .. } | |
| pub fn close_with_reason(self, code: u16, reason: Option<&str>) -> Result<(), CloseError> { .. } |
| pub struct WebSocket { .. } | ||
|
|
||
| impl Stream for WebSocket { | ||
| type Item = Result<Message, ConnectionError>; |
There was a problem hiding this comment.
This is for outgoing messages, right?
| The main alternative here is that we could use a bespoke model for the futures websocket, rather than | ||
| using standard traits (Stream + Sink). I'm not sure exactly which would be preferable. |
There was a problem hiding this comment.
I would prefer to keep the API using standard traits.
| The main alternative here is that we could use a bespoke model for the futures websocket, rather than | ||
| using standard traits (Stream + Sink). I'm not sure exactly which would be preferable. | ||
|
|
||
| ## Unresolved Questions |
|
|
||
| ```rust | ||
|
|
||
| /// An incoming websocket message. |
There was a problem hiding this comment.
It seems this is also used for outgoing messages (see Stream impl comment) so this is a misleading doc comment?
|
Another thing: @derekdreery, can you create an issue for this RFC with the changes. See #136 for details |
This is a first draft of a websocket API. I don't think it's ready to merge yet, but I wanted to start the conversation.
rendered