Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use builder pattern to construct WebRtcSocket #158

Merged
merged 11 commits into from
Mar 18, 2023

Conversation

garryod
Copy link
Collaborator

@garryod garryod commented Mar 15, 2023

Had a go at creating a builder for the WebRtcSocket to replace WebRtcSocketConfig, this should make things a touch easier when it comes to constructing sockets with numerous channels.

Also added a helper to the ggrs feature to allow users to more easily add an appropriate channel.

@garryod garryod changed the title Use builder pattern to construct WebRtcSocket Use builder pattern to construct WebRtcSocket Mar 15, 2023
Copy link
Collaborator

@simbleau simbleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add WebRtcSocket::builder() -> WebRtcSocketBuilder

@garryod
Copy link
Collaborator Author

garryod commented Mar 16, 2023

Please add WebRtcSocket::builder() -> WebRtcSocketBuilder

Good idea! Have used the same arguments as WebRtcSocketBuilder::new, but not sure this feels quite right

examples/bevy_ggrs/src/main.rs Outdated Show resolved Hide resolved
matchbox_socket/src/ggrs_socket.rs Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Show resolved Hide resolved
Copy link
Collaborator

@simbleau simbleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant.

matchbox_socket/src/ggrs_socket.rs Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Outdated Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Outdated Show resolved Hide resolved
@johanhelsing
Copy link
Owner

Look good now :) I only had a doc nit and a question.

garryod and others added 2 commits March 18, 2023 10:17
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
@johanhelsing
Copy link
Owner

Oh, I'd still like some dead-simple convenience methods that doesn't involve creating a builder.

let socket = WebRtcSocket::new_unreliable("wss://example.com");

As opposed to:

let socket = WebRtcSocket::builder("wss://example.com")
    .add_unreliable_channel()
    .build();

Internally, it would just call the builder.

I want to keep simple/common usage really simple as far as possible.

@garryod
Copy link
Collaborator Author

garryod commented Mar 18, 2023

Oh, I'd still like some dead-simple convenience methods that doesn't involve creating a builder.

Done! Do you think we should use these or the builder in the examples?

@johanhelsing
Copy link
Owner

Oh, I'd still like some dead-simple convenience methods that doesn't involve creating a builder.

Done! Do you think we should use these or the builder in the examples?

Hmmm.. I think it would be good to have at least one example that shows how to set up multiple channels, but in our current examples i think the simple versions are probably better.

@garryod
Copy link
Collaborator Author

garryod commented Mar 18, 2023

Hmmm.. I think it would be good to have at least one example that shows how to set up multiple channels, but in our current examples i think the simple versions are probably better.

That seems reasonable to me, a mutli-channel example would definitely be good to have

@simbleau
Copy link
Collaborator

Not really convinced, wouldn't we have to add a lot of API: ip/hostname, port, which ws and wss, path and query string building? Only to convert it to a string since that's what the browser/async_tungstenite wants anyway... I think we should just wrap WsErr::InvalidUrl (and the native variant) into one of our errors when connecting.

Ok, I think it clicked for me when you said "that's what the browser wants anyways".

Fair point.

Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let's get the money! 🎉

@johanhelsing johanhelsing merged commit 7e0e387 into johanhelsing:main Mar 18, 2023
matchbox_socket/src/ggrs_socket.rs Show resolved Hide resolved
matchbox_socket/src/ggrs_socket.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants