-
Notifications
You must be signed in to change notification settings - Fork 943
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(yamux): future-proof public API #3908
Conversation
… into feat/harden-yamux-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Looks good to me overall. Just one suggestion.
/// The [`futures::stream::Stream`] of incoming substreams. | ||
incoming: S, | ||
incoming: BoxStream<'static, Result<yamux::Stream, yamux::ConnectionError>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Box
the stream of streams? Not boxing would also resolve the need for PhantomData
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just inlined what we had before.
This and the phantom data will go away once update to latest yamux. I only added it to make sure it is a compatible API change later :)
into_stream
returns an impl Trait
. I guess we could inline the unfold operation and store an Unfold
type?
Do you feel strongly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the Unfold
type is nameable at all because it contains an async block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the phantom data will go away once update to latest yamux.
Great. Don't feel strongly. Let's keep as is.
Description
With this set of changes, we prepare the public API of
libp2p-yamux
to be as minimal as possible and allow for upgrades of the underlyingyamux
library in patch releases.Related: #3013.
Notes & open questions
Change checklist