Docs for webrtc_socket/messages.rs#543
Docs for webrtc_socket/messages.rs#543Craig-Macomber wants to merge 2 commits intojohanhelsing:mainfrom
Conversation
johanhelsing
left a comment
johanhelsing
left a comment
There was a problem hiding this comment.
Thanks, getting the protocol well-documented is important in order to make the project approachable for newcomers.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| /// Events go from signaling server to peer | ||
| /// Events which go from signaling server to peer. |
There was a problem hiding this comment.
Actually the wording was intentional here to make it clear that events always go from server to peer.
There was a problem hiding this comment.
With my phrasing, at least to me, this says that a "PeerEvent" is a kind of event which goes from server to peer, and I think thats accurate. There are lots of other kinds of events (For example RTCDataChannelEvent) and this sentence is clarifying that those other events are not PeerEvents.
I think the old phrasing seems like its trying to define the english word event with a really narrow definition, or clarify the server is able to pass events to the peer without stating that those events are PeerEvents.
I do think this would make sense:
PeerEvents go from signaling server to peer.
Given that additional justification, would you prefer that, what I have in the PR, or the original phrasing?
I'm happy to update this and the one below to match based on your preference.
| pub type PeerEvent = matchbox_protocol::PeerEvent<PeerSignal>; | ||
|
|
||
| /// Requests go from peer to signaling server | ||
| /// Requests which go from peer to signaling server. |
| /// safety and clarity at this layer. It may be preferable to instead include the structured | ||
| /// data here as more specific Rust types, but this would be a breaking protocol change, and |
There was a problem hiding this comment.
This looks it should probably be an regular (implementation) comment and not a doc comment.
The reason for using string was that most documentation for webrtc state that you should treat this payload black boxes. I was thinking that trying to parse it would only lead to maintanance burden for no benefit.
There was a problem hiding this comment.
most documentation for webrtc state that you should treat this payload black boxes
That can't apply here, where we have to encode and parse this string in both the native and wasm socket implementations. It makes sense for the server however (which doesn't use this type).
What can (and should be opaque here) is the candidate string and stuff like that. The object that contains that property however doesn't really seem like something that needs to be opaque on the socket end.
That said, when I wrote this, I intended the audience to be maintainers of the wasm and native socket implementations (as thats the code interfacing with this type), and thus this being a doc comment makes sense. I missed that these types are exported from the crate for use in custom signalers which also should treat this as opaque.
Because of that making this structured data likely doesn't make sense and some other solution should be done to dedupe the logic between wasm and native, and provide them with stronger typing to keep them aligned. I have adjusted the comment accordingly.
2 participants
Previously it was not particularly clear that nothing in messages.ts was used for actual peer to peer messages, and its specific to signaling. I have added some top level docs to make this clear, and filling in the lower level docs to make it clear what the content of the various strings are.
These details were determined by inspecting the code. If there is an intention for this to be more general than how the current native and wasm implementation use them, then these detail might be wrong, but I think the intention is that these always contain the strings as I have described them.