warp icon indicating copy to clipboard operation
warp copied to clipboard

Make the websocket Message easier to work with

Open FSMaxB opened this issue 3 years ago • 5 comments

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: https://github.com/seanmonstar/warp/blob/9e74ff98e0f314879c29cf761a372840606bd585/src/filters/ws.rs#L267-L274

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 avatar Sep 27 '21 11:09 FSMaxB

Rebased to fix formatting

FSMaxB avatar Sep 27 '21 12:09 FSMaxB

Hi, and thanks for your interest! There is already https://github.com/seanmonstar/warp/pull/821 on the pipeline

jxs avatar Sep 30 '21 20:09 jxs

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.

FSMaxB avatar Sep 30 '21 20:09 FSMaxB

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 avatar Sep 30 '21 20:09 jxs

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

mvolfik avatar Mar 30 '22 10:03 mvolfik