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

docs(websocket): showcase wrapping dns and tcp transport #3385

Merged
merged 5 commits into from
Jan 30, 2023

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Jan 25, 2023

Closes #3330

@melekes
Copy link
Contributor Author

melekes commented Jan 25, 2023

Re #3330 (comment) providing Default on the transport, we can do so for tokio, but how to do it for async-io since dns::DnsConfig::system returns future? Is there a async Default trait?

impl Default for WsConfig<T: Transport> {
    #[cfg(feature = "async-io")]
    fn default() -> Self {
        WsConfig {
            transport: dns::DnsConfig::system(
                tcp::async_io::Transport::new(tcp::Config::default()),
            )
            .await
            .unwrap(),
        }
    }

    #[cfg(feature = "tokio")]
    fn default() -> Self {
        WsConfig {
            transport: dns::TokioDnsConfig::system(
                tcp::tokio::Transport::new(tcp::Config::default()),
            )
        }
    }
}

cc @thomaseizinger

mxinden
mxinden previously approved these changes Jan 26, 2023
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Ready to merge from my end.

Thanks @melekes for making this easier for future users!

Will wait a bit to give @thomaseizinger a chance to reply as well.

@mxinden mxinden changed the title [transports/websocket] add 2 doc examples docs(websocket): showcase wrapping dns and tcp transport Jan 26, 2023
@mxinden mxinden changed the title docs(websocket): showcase wrapping dns and tcp transport docs[websocket]: showcase wrapping dns and tcp transport Jan 26, 2023
@mxinden mxinden changed the title docs[websocket]: showcase wrapping dns and tcp transport docs(websocket): showcase wrapping dns and tcp transport Jan 26, 2023
@thomaseizinger
Copy link
Contributor

There is no async default so we can't do that unfortunately.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I would still be in favor of dedicated constructors that take the necessary inner configs:

impl WsConfig {
    async fn new_wss(dns_config: ..., tcp_config: ...) -> Self {}
    fn new_ws(tcp_config: ...) -> Self {}
}

The examples can then move to the docs of these functions.

Happy to merge this too as it is better than what we have today.

Thanks @melekes !

@melekes
Copy link
Contributor Author

melekes commented Jan 27, 2023

I would still be in favor of dedicated constructors that take the necessary inner configs:

It just feels too cumbersome when I add 4 new methods (2 fn and 2 features: tokio and async-io). I'd be in favor of just merging as is.

@thomaseizinger
Copy link
Contributor

I would still be in favor of dedicated constructors that take the necessary inner configs:

It just feels too cumbersome when I add 4 new methods (2 fn and 2 features: tokio and async-io). I'd be in favor of just merging as is.

Ah yes, I forgot that we'd have to do one per executor.

thomaseizinger
thomaseizinger previously approved these changes Jan 29, 2023
@mergify mergify bot dismissed stale reviews from mxinden and thomaseizinger January 30, 2023 12:10

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 47c1d5a into libp2p:master Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is secure WebSocket transport even working?
3 participants