-
Notifications
You must be signed in to change notification settings - Fork 1k
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(libp2p): deprecate mplex #3689
Conversation
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! I've done a first pass :)
Let me know if you have any questions.
254d6c3
to
6f87618
Compare
This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏 |
This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏 |
@@ -10,4 +10,4 @@ async-std = { version = "1.12", features = ["attributes"] } | |||
async-trait = "0.1" | |||
env_logger = "0.10" | |||
futures = "0.3.28" | |||
libp2p = { path = "../../libp2p", features = ["async-std", "dns", "kad", "mplex", "noise", "tcp", "websocket", "yamux"] } |
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.
You need to undo this line to make the IPFS integration test work again.
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.
@thomaseizinger Done, maybe for future PR: you said in the comments above that you didn't want to use deprecated modules like this in the examples
. In this context, shouldn't mplex
be replaced by yamux
here?
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.
Yes correct. In this case, we still use development_transport
for this example. I'd like to change how this function is being used but we still need to hash out a design. See #3657.
The option here would either be to use inline development_transport
into the example or use the deprecated feature here. To keep the scope of the PR small, I'd say using it here is fine enough. We don't explicitly show the use of mplex
in here so I don't think it is that bad. With the next breaking change, we will remove mplex
from the development_transport
and the world will be better :)
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 your contribution and patience!
I'll queue this for merging.
This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏 |
Approvals have been dismissed because the PR was updated after the send-it
label was applied.
Description
mplex
module (no flow control and no streams limit) is deprecated to encourageyamux
usage.Resolves #3677.
Change checklist