Fix the protocol abstraction so different protocols can be supported #48
Conversation
BrennanConroy
force-pushed
the
brecon/protocol
branch
from
4ac367e to
2d09c28
Compare
BrennanConroy
force-pushed
the
brecon/protocol
branch
from
2d09c28 to
a2e1e4e
Compare
src/signalrclient/hub_protocol.h
Outdated
| virtual std::string write_message(const signalr::value&) const = 0; | ||
| virtual std::vector<signalr::value> parse_messages(const std::string&) const = 0; | ||
| virtual std::string write_message(const hub_message_base*) const = 0; | ||
| virtual std::vector<std::unique_ptr<hub_message_base>> parse_messages(const std::string&) const = 0; |
There was a problem hiding this comment.
Not a fan of this, but it's the only (clean) way I could think of passing the message type back to the connection.
There was a problem hiding this comment.
I can't think of anything better at the moment, but can we rename hub_message_base to hub_message?
| @@ -20,12 +20,13 @@ namespace signalr | |||
| public: | |||
| explicit default_websocket_client(const signalr_client_config& signalr_client_config = {}) noexcept; | |||
|
|
|||
| void start(const std::string& url, transfer_format format, std::function<void(std::exception_ptr)> callback); | |||
| void start(const std::string& url, signalr::transfer_format format, std::function<void(std::exception_ptr)> callback); | |||
There was a problem hiding this comment.
A WebSocket can switch between text an binary in the same connection, right? Could we make the transfer_format an argument to send and receive rather than start? I know SignalR doesn't need to ever switch mid-connection, but it seems like a cleaner to match what the protocol allows.
There was a problem hiding this comment.
A WebSocket can, but SignalR can't/won't.
In the other clients we pass the format into start as well
https://github.com/dotnet/aspnetcore/blob/c0a2024a54c848c85ddff460dbf3fb12b7898f70/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs#L112
There was a problem hiding this comment.
I know the other clients do it this way. I never liked that about the other clients either. Let's make this one a bit more flexible.
There was a problem hiding this comment.
Updated, with this change the transport has to figure out the format itself in the receive loop. At least for WebSockets this is fine
Pre-req for MessagePack support