#13: scoped rooms #19
Conversation
|
disclaimer:
|
matchbox_server/src/signaling.rs
Outdated
|
|
||
| #[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
johanhelsing
left a comment
johanhelsing
left a comment
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
| } | ||
|
|
||
| #[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
| } | ||
| } | ||
| #[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
johanhelsing
left a comment
johanhelsing
left a comment
There was a problem hiding this comment.
The matchbox_demo example needs to be updated now (to use the new next API), and I had a couple of tiny nits, but otherwise, this looks really great!
It's really awesome to have this so well tested with all the corner-cases we discussed.
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. |
johanhelsing
left a comment
johanhelsing
left a comment
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 ;) |
2 participants
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.