-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
cce6793
to
5988cc3
Compare
Are you sure the changelog does not need updating? |
} | ||
} | ||
None | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { |
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.
Check the commit messages to learn why this is gone.
/// Waiting for a request from the remote. | ||
WaitingMessage { stream: Frames<TSubstream> }, | ||
WaitingMessage { stream: Pin<Box<Frames>> }, |
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.
We have to Pin
the stream to a specific location in memory.
The current code here does what async/await would do under the hood: It represents the state as an enum and each variant is a check (await) point.
async functions automatically use Pin
if necessary.
We have to use it here, to be able to access the poll
APIs on the stream.
We have to depend on libp2p in cnd anyway, so the compile times are not really affected by this and simplifies the versioning by removing a dependency.
Unfortunately, I had to trash all the tests in libp2p-comit because I just couldn't get them to work. The e2e tests work though and we are going to trash this communication protocol soon anyway. Hence, in the interest of not wasting more time, I deleted them. On the upside, libp2p 0.16 offers a lot of very nice things: 1. The TSubstream type variable is gone which reduces the overall verbosity of the code. 2. The update to futures 0.3 and async/await is obviously nice. 3. We no longer need to print the errors of a ProtocolHandler ourselves, this is now done in the Swarm (on level TRACE).
5988cc3
to
854e972
Compare
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.
Thank you for doing this, Thomas 😄
LGTM and makes sense to give up on those tests if they would have to go away soon anyway.
Can you comment on this note in the original issue?
Note that the goal here is to avoid bringing async-std into our dependency tree (check with cargo tree). It seems like it is not possible to completely achieve that as mdns still depends on async-std without providing a feature-flag.
Sure! |
bors r+ |
Build succeeded |
Resolves #1941.