#13: scoped rooms #19
Conversation
|
disclaimer:
|
matchbox_server/src/signaling.rs
Outdated
| @@ -37,11 +37,22 @@ type PeerRequest = matchbox::PeerRequest<serde_json::Value>; | |||
| type PeerEvent = matchbox::PeerEvent<serde_json::Value>; | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| pub(crate) enum RequestedRoom { | |||
| pub(crate) enum RoomType { | |||
There was a problem hiding this comment.
I renamed that so it can easily be embedded in a more customizable struct RequestedRoom
There was a problem hiding this comment.
Hey, sorry, I didn't actually see the pr earlier today when I commented on #13.
As discussed I feel like having the next_n functionality an optional query parameter makes a lot of sense, so the API would be
wss://match.example.com/{id}?next={n}
Especially if we'd want to tuck on other features as well in query parameters. There isn't much reason to treat the next_n rooms like a special citizen.
One question though, is what we do if one peer tries to connect to
wss://match.example.com/abcd?next=2
while somebody else tries to connect to
wss://match.example.com/abcd
I guess we'd have to make the query parameter part of the key for the room? Idk, I'm open to suggestions here.
Yeah I think a I'm working on it :) |
…ging together and
| @@ -453,8 +463,159 @@ mod tests { | |||
| } | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn match_different_id_same_next() { | |||
There was a problem hiding this comment.
here we check
- player a -> scope_1?next=2
- player b -> scope_2?next=2
- player c -> scope_1?next=2
- player d -> scope_2?next=2
| } | ||
| } | ||
| #[tokio::test] | ||
| async fn match_same_id_different_next() { |
There was a problem hiding this comment.
here we check
- player a -> scope_1?next=2
- player b -> scope_1?next=3
- player c -> scope_1?next=2
- player d -> scope_1?next=3
- player e -> scope_1?next=3
| @@ -452,9 +459,212 @@ mod tests { | |||
| _ = &mut timeout => {} | |||
| } | |||
| } | |||
| #[tokio::test] | |||
| async fn match_pair_and_other_alone_room_without_next() { | |||
There was a problem hiding this comment.
Here we check:
- player a -> scope_1?next=2
- player b -> scope_1
- player c -> scope_1?next=2
matchbox_server/src/signaling.rs
Outdated
| let mut client_a = warp::test::ws() | ||
| .path("/room_name?next=2") | ||
| .handshake(api.clone()) | ||
| // .handshake(ws_echo()) |
There was a problem hiding this comment.
nit: Should probably get rid of these commented out lines
There was a problem hiding this comment.
other nit, that PR adds a lot of duplicatd code for tests ; I guess it's usual so why not, but it might be a good time to split in another file for tests ?
There was a problem hiding this comment.
I feel that in tests it's far better to err on the side of simple rather than dry. imo it's only worth it if the test becomes easier to read and debug.
There was a problem hiding this comment.
I'm ok with repetition in tests 👍.
I especially want to be clear that signalling.rs sees its line count going from 460 to 650, so it might be a good time to consider moving tests out of that file.
|
I updated matchbox_demo ; it worked fine locally 🎉 . I didn't update documentation because I didn't find any place where the "next" logic is mentionned, I guess most information is from your blog and the example. I conclude it probably belong to another issue or Pull Request to add documentation. |
There was a problem hiding this comment.
I updated matchbox_demo ; it worked fine locally tada .
I didn't update documentation because I didn't find any place where the "next" logic is mentionned, I guess most information is from your blog and the example. I conclude it probably belong to another issue or Pull Request to add documentation.
Yeah, seems like it's not documented in the repo.
It'd probably be a good idea to add some api docs.
Great work!
|
Merging. I think moving tests to a separate file is a good idea, and documentation as well. More PRs always welcome ;) |
Implements #13
This also allows to scope rooms with id.
I adapted a test to use scopes, but we might want to create separate tests rather than changing the existing one.