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

feat(iroh-net): Own the public QUIC API #2279

Merged
merged 6 commits into from
May 10, 2024
Merged

Conversation

flub
Copy link
Contributor

@flub flub commented May 10, 2024

Description

This export a lot of quinn types directly from
iroh-net::magic_endpoint. These are all the types we need to interact
with iroh-net in our own code. The goal is that users should not need
to figure out how to add their own (iroh-)quinn dependency to use
iroh-net, instead all types should be provided by iroh-net.

Not all APIs are re-exported however, to avoid exporting too much.
Hopefully what iroh itself needs is a reasonable indication of what is
needed, we can always add more.

Breaking Changes

Notes & open questions

  • This ducks re-writing the iroh-blobs examples.
  • This leaves quinn as a dependency of iroh itself, this is still needed because quic-rpc likewise does not provide all the needed types in it's public API. We should probably do a similar effort there.

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.
  • All breaking changes documented.

This export a lot of quinn types directly from
iroh-net::magic_endpoint.  These are all the types we need to interact
with iroh-net in our own code.  The goal is that users should not need
to figure out how to add their own (iroh-)quinn dependency to use
iroh-net, instead all types should be provided by iroh-net.

Not all APIs are re-exported however, to avoid exporting too much.
Hopefully what iroh itself needs is a reasonable indication of what is
needed, we can always add more.
@flub flub requested a review from dignifiedquire May 10, 2024 13:06
@flub flub enabled auto-merge May 10, 2024 13:13
@flub flub disabled auto-merge May 10, 2024 13:13
@flub flub changed the title feat(iroh-net): Export many more quinn types feat(iroh-net): Own the public QUIC API May 10, 2024
iroh-cli/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

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

really important change to get in soon. Any reason to handle the typing and docs in the qualified way vs doing imports?

/// [`Connection`]: iroh_net::magic_endpoint::Connection
pub async fn handle_connection(
&self,
conn: iroh_net::magic_endpoint::Connection,
Copy link
Contributor

Choose a reason for hiding this comment

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

we already do a bunch of iroh_net imports in these files. Why not do the same and keep the docs and params simply referring the import? The new state is a tad too verbose for my liking. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with a direct replacement as I was not editing the local code: if quinn::SomeThing was used I replaced it without imports as well. It is true the new full name is a lot longer. It will probably also change to iroh_net::endpoint sometime soon as well which is at least a bit shorter.

I will attempt some cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

290e447 is more aggressive with imports, hope this is better

Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

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

LGTM.

Related, do we not export these types in the public api's of the individual crates? should we? not sure on either

@dignifiedquire dignifiedquire added this pull request to the merge queue May 10, 2024
Merged via the queue into main with commit b62e904 May 10, 2024
22 checks passed
@flub flub deleted the flub/net-no-quinn branch May 13, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants