Return the result of send()

[hhxsv5]

No description provided.

[mbonneau]

@hhxsv5 - Thanks for this catch. I will tag a new release shortly.

[hhxsv5]

@mbonneau Nice! You are welcome.

[clue]

@hhxsv5 What is the motivation for this change?

It's my understanding the current API mimics Browser's WebSocket API, which doesn't return anything either, no?

[hhxsv5]

I just want to know if the send operation is successful or if there are any errors. like gorilla/websocket @clue

[clue]

The underlying write() method does not return a "succesful" status. It only returns whether the given data could be added to the buffer or whether the client should pause sending data before the buffer is drained, see also https://github.com/reactphp/stream#write.

I understand where you're coming from and it looks like your request would be best addressed when #88 is resolved.

In the meantime, I would argue that this patch is counterproductive.

[mbonneau]

@clue Isn't letting the user know if they should pause sending valuable enough to return?

[hhxsv5]

@clue I understand your concerns, it doesn't matter. If the return value does not represent the actual result of the call, specify it in the comment or document. Here I recommend returning the value of write.

[clue]

@mbonneau I agree that this provides some value, but I'm not sure this is how higher-level implementations actually (ought to) use this.

It's my understanding the current API mimics Browser's WebSocket API, which doesn't return anything either. In particular, the WebSocket::send() usually buffers by default and will close the socket with a fatal error if the buffer size is exceeded. Depending on how you want this project's API to be used, this may or may not make sense here as well. I agree that this is something that should be discussed (perhaps outside of this ticket).

IMHO returning a boolean in this higher-level implementation is misleading and might give a false sense of "succesful" writes. This being an async implementation means that "unsuccessful" writes may only be detected at a later time, e.g. when the socket reports an error etc.

[mbonneau]

@clue I was considering that this may be useful for flow control - but to do that we would need to know when it is ok to send again, which the API gives no access to. This makes that return value less important.

I could go either way as I don't think there is anything more useful to return.

[clue]

@mbonneau No objections, didn't mean to block this change. It's still my understanding that this return value might be confusing for people not intimately familiar with the underlying stream abstraction, but I guess some good documentation might help mitigate this 👍