-
Notifications
You must be signed in to change notification settings - Fork 46
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
fix: Rename Protocol::WebRTC
to Protocol::WebRTCDirect
#78
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.
👍
@@ -202,7 +202,7 @@ impl<'a> Protocol<'a> { | |||
} | |||
"p2p-websocket-star" => Ok(Protocol::P2pWebSocketStar), | |||
"p2p-webrtc-star" => Ok(Protocol::P2pWebRtcStar), | |||
"webrtc" => Ok(Protocol::WebRTC), | |||
"webrtc-direct" => Ok(Protocol::WebRTCDirect), |
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 must say having both webrtc-direct
and p2p-webrtc-direct
is confusing, but I guess nothing we can do about it other than adding clear documentation.
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 could as well deprecate p2p-webrtc-direct
. I am not aware of anyone using it. Though intuitively, that decision should happen on https://github.com/multiformats/multiaddr.
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.
Could we first release a version where we accept parsing as /webrtc-direct
and not rename the variant, thus making this a non-breaking change?
I'd like to batch some breaking changes in rust-multiaddr
together, e.g. #77 and #73 but those are not ready yet. Especially #73 needs a breaking change in libp2p-identity
first which implies a round of breaking changes to libp2p
.
On first thought this is a great idea. Though I am not sure there are no hidden surprises with this strategy. E.g. the guarantee of round-trip parsing ( Breaking change:
Non-breaking change:
Thoughts @thomaseizinger? |
Friendly ping @thomaseizinger. What are your thoughts here? |
Didn't submit my comment 🤦♂️ |
What we'd need to work out is how exactly we are going to introduce the other We definitely need to log a deprecation warning if we are now adding the 2nd representation. Then in a breaking change, we can rename the variant and add the "new" How big is the footgun actually? |
The only problem I can think of is that in the "breaking" change where we add the new (browser-to-browser) Ideally, semantic breaking changes are also compile-time breaking changes but I'd say with sufficient documentation, we can also deviate from that. I've pushed multiformats/rust-multihash#272 forward a lot so I'd love if the next breaking change can already include that (and thus be the last one in a while). |
If we move forward with this, it would have to be done as a patch-release from |
Pull request for the non-breaking change: #84 |
/webrtc
to /webrtc-direct
Protocol::WebRTC
to `Protocol::WebRTCDirect
Protocol::WebRTC
to `Protocol::WebRTCDirectProtocol::WebRTC
to Protocol::WebRTCDirect
… into webrtc-direct
With #84 released as |
I seem to only be able to request a review from one of the two of you. |
@@ -1,9 +1,15 @@ | |||
# 0.18.0 [unreleased] | |||
# 0.18.0 - unreleased | |||
|
|||
- Add `WebTransport` instance for `Multiaddr`. See [PR 70]. | |||
- Disable all features of `multihash`. See [PR 77]. | |||
- Mark `Protocol` as `#[non_exhaustive]`. See [PR 82]. |
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 means we can then later add WebRTC
in a non-breaking way 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.
Correct
Co-authored-by: Thomas Eizinger <[email protected]>
@mxinden when is this v0.18 planned for release so that we can use the new naming without patches? |
I'd first like to get a release of |
I've created a milestone for |
See multiformats/multiaddr#150 (comment) for context.
@melekes @thomaseizinger can you give this a review?