handle first argument improperly descriptive

[OvermindDL1]

Specifically things like def handle(:text, message, _state) do imply it is a textural format, where websockets are a binary stream and it should probably be exposed as such. Probably just :input or something would be more properly descriptive of the message.

As well as returning just a state as a return option from handle is ambiguous with the other options and should be properly tagged, probably by following the GenServer standard style for not returning something, same with the other formats, they should probably all follow the style of GenServer's.

[satom99]

From Phoenix' handling, we can observe Cowboy's semantics and how these are tackled - the {opcode, payload} tuple is broken down into separate arguments and applied to a handler callback. These arguments are the same that handle is receiving.

Therefore, I would say the discussion should be at whether to turn both arguments into a tuple again and hence follow GenServer's handle_call syntax. Although I'm not really keen about doing this as it would just require introducing tuple-matching over the simplicity gained with split arguments.

Aside and on to returning, I don't quite like the idea of following GenServer's syntax. Replying with {:reply, {:text, "this is a reply"}, state} feels heavy and tedious to maintain readable as more callback matches are added. The same would apply to not replying, a {:noreply, state} tuple would be required everywhere. There also are some calls where you might not alter the state at all, thus having to return it back all the time is not good either. I do agree that his may be somewhat ambiguous, but I think that the benefits are worth the trade. The same applies to the send syntax.

Now, I assume you mention :input because there's no :binary example, but keep in mind it is being dispatched. From this though, I would like to introduce another thing to discuss; there is a :closed event that is sent through the same callback. Would it make sense to send :input and :event as a first argument to differentiate or would this be too overkill? I am up for the latter - I don't think that's necessary.

I understand standardization is a concern though, and that nor should I be right. Let's discuss! 😄

[OvermindDL1]

I definitely prefer the 'heavier' genserver form, the unambiguousness of it makes it easy to both read and grep for (I quite often search based on reply and noreply for example), plus it opens the ability to add in timeout and hibernation support like genserver's as well. Readability is paramount.

Wait, cowboy actually dispatches text and binary separately, that makes no sense on a streaming socket type like a websocket... o.O

And I'd say separating the inputs and :event as overkill, they are still the standard message headers. :-)

And yes, standardization makes things easier to learn, use, and let's people re-use existing code more often as well. :-)

[satom99]

Implementing a custom version of this module would be required in the case we want to be able to receive all opcodes (as Phoenix only dispatches text and binary) and add support for hibernation.

I am still unsure on whether following GenServer's return semantics would be a better idea. If a modification of the previously stated module is included, hibernation could be enabled by default - assuming there's no downside to this(?). You mention grep'ing for :noreply, but do keep in mind it is perfectly fine to return an atom that can be grep'd for later on - as atoms are ignored. We both agree standardization is indeed important, but I still find the current syntax a lot cleaner and easier to read over the proposed semantics - in spite of the possible ambiguity of course, which I guess could be avoided with better examples(?).

Looking forward to a reply, specifically on implementing the mentioned module and hibernation. 😋

[OvermindDL1]

Hibernation has a big downside! Flushing the GC on every call can be quite expensive and a hibernation is one of the only ways to force an entire GC of an entire Actor, so you generally only want to call it when you will not expect to be called again for 'a while'.

I'd still follow the GenServer API, the hibernation and timeout functionality is very useful plus it is entirely unambiguous, which is always a good thing. ^.^

[satom99]

Given that this was initially thought to act as a bridge to Cowboy and to provide all of its functionality, I have to agree that following a standardized syntax is a must. Keeping the current simplified syntax would only make sense in a more specific construction, but not in what's meant to be the base for whatever the end developers' needs might be.

Hereby, we should decide whether to follow GenServer's or Cowboy's internal syntax. I would personally go for the latter, as this way we let the developer interact with Cowboy directly and prevent us from having to tackle any possible future syntax changes. Although the former might make things a bit nicer to handle - as well as to allow the implementation of some common goodies, such as GenServer's stop syntax.

I kind of liked the idea of keeping a cleaner syntax, but this is the most reasonable thing to do. 😜