search

Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

WebSocket text vs. binary #50

Closed Tectu closed 3 years ago

Tectu commented 3 years ago

We should provide the user with the option of controlling whether message opcodes are set as text or binary. See https://www.boost.org/doc/libs/develop/libs/beast/doc/html/beast/ref/boost__beast__websocket__stream/binary/overload1.html

Tectu commented 3 years ago

@0x00002a Do you think that websocket::connection should accept this as a constructor or even template parameter or rather just providing wrappers towards the underlying websocket::stream::set_binary() and websocket::stream::binary() (introduced in branch feature/ws_binary)?

0x00002a commented 3 years ago

I was thinking the same thing :p. I think its probably better to just wrap it though, as I understand it malloy is trying to be a light a wrapper around websockets as possible while still not exposing a bunch of implementation details. If I understand correctly, the "binaryness" of a stream can change at any time so it make sense to let the user do that. If its a one-time or initialisation thing though I think it should probably be a template argument ("binary_connection" or something).

As a side note, is this library going to be using set_ as prefix to setters? I'm fine with it style wise, just used to the no prefix or suffix version (just name for getter and name with param for setter)

Tectu commented 3 years ago

Seems like we fully agree on this. From the specs I do understand that binaryness may change at any time. I couldn't come up with a scenario where that would be necessary/beneficial/required but that doesn't mean that it doesn't make sense. As you pointed out malloy is supposed to be a lightweight wrapper for ease of use so if the option is already there we should just pass it through. It does kind of hurt providing connection::set_binary() and connection::binary() wrappers. That's just dead code that is cluttering everything up. Looking at the overall situation this does seem to be the best way to go here.

As a side note, is this library going to be using set_ as prefix to setters? I'm fine with it style wise, just used to the no prefix or suffix version (just name for getter and name with param for setter)

Yep, malloy is using set_ prefixes. I did notice that you didn't do that in at least one function but didn't want to be a nazi as this library is in early development anyway. Of course this is also a personal opinion/decision. A couple of years ago I wasted some time when working with 3rdparty code which in turn used some dependency with non-set_-prefixed setters. It was an unfortunate combination of a getter accepting additional arguments to pass out more information. Ever since then I decided (for myself, not for other humans!) that setters shall be prefixed with set_ (or set if working with snakeCase).