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

Make the websocket Message easier to work with #903

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FSMaxB
Copy link
Contributor

@FSMaxB FSMaxB commented Sep 27, 2021

Currently the Message type is a bit hard to work with as can be seen here: https://github.com/communityvi/communityvi/blob/26bfbe16c8c4dc46545edeebf96d2b3aed8887ab/communityvi-server/src/utils/websocket_message_conversion.rs#L4-L33

While looking at the code I also happened to notice the comment about non-exhaustive enum:

warp/src/filters/ws.rs

Lines 267 to 274 in 9e74ff9

/// A WebSocket message.
///
/// This will likely become a `non-exhaustive` enum in the future, once that
/// language feature has stabilized.
#[derive(Eq, PartialEq, Clone)]
pub struct Message {
inner: protocol::Message,
}

It would of course be best for usability/interoperability if warp used tungstenite::protocol::Message directly. I assume the opaque Message type was introduced to allow for changes in the underlying implementation, without breaking warp's API?

This pull request contains 2 commits:

  1. Convert Message into a non-exhaustive enum without breaking the existing API
  2. Add From for tungstenite::protocol::Message in both directions. This doesn't break the API, but it exposes the respective version of the tungstenite::protocol::Message, making any future update of the tungstenite dependency a potentially breaking change. If that is undesired, I can remove this second commit.

@FSMaxB FSMaxB force-pushed the non-exhaustive-websocket-message branch from a9f7ee5 to 1ce802b Compare September 27, 2021 12:19
@FSMaxB
Copy link
Contributor Author

FSMaxB commented Sep 27, 2021

Rebased to fix formatting

@jxs
Copy link
Collaborator

jxs commented Sep 30, 2021

Hi, and thanks for your interest! There is already #821 on the pipeline

@FSMaxB
Copy link
Contributor Author

FSMaxB commented Sep 30, 2021

Sorry for not doing due diligence and checking for existing pull requests.

How should I proceed? Close this in favor of the existing pull request? I've noticed that there hasn't been any progress in the original pull request for several months.

@jxs
Copy link
Collaborator

jxs commented Sep 30, 2021

no worries! :) yeah I was just noticing that as well, and this one already addresses my review on the other and uses From<protocol::Message> which is cleaner LGTM!

@jxs jxs added the waiting-on-reviewer Awaiting review from the assignee but also interested parties. label Sep 30, 2021
@mvolfik
Copy link
Sponsor

mvolfik commented Mar 30, 2022

Sorry for bumping, but is there anything I could do to help get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-reviewer Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants