angular2-websocket
angular2-websocket copied to clipboard
afrad
send returning cold Observable requires subscription.
There seems to be a breaking change in how send is implemented. It now returns an cold Observable, created using Observable.create which requires a subscribe in order to fire.
I believe your ultimate goal is to link the observable to the actual successful/failure of the websocket send. However, currently as it stands, it doesn't do that.
However, this change has introduced a new hassle: every call to send now needs a subscription in order to work.
For example:
Previous: wsock.send(someObj);
Now: wsock.send(someObj).subscribe();
Would you consider creating a hot observable instead? It is not a great idea since hot observables h ave their own overheads.
The best solution might be to have two different send methods? That is because maybe not many people will care whether the message is successfully sent or not.
I'm also wondering what the purpose of the send queue is... it looks like you're trying to keep messages through disconnections of the websocket. However, most browsers' WebSocket implementation already has an internal queue that doesn't get lost even through disconnect-reconnect cycles. Therefore, I would think the send queue is duplicated and unnecessary...
OK, I see it now. You're establishing new WebSocket connections when reconnecting. That explains your usage of the send queue, but I'm afraid it still may not help you much...
There is no easy way to check if the message is sent internally by the WebSocket, or simply queued to be sent. Your implementation simply dumps the entire queue into the WebSocket's internal queue, which when the connection breaks, they are lost anyway because they don't stay in your send queue for too long. You call fireQueue every time a send call is subscribed to.
The only way, I can think, to handle this is to check bufferedAmount on the websocket. If there is data buffered, and if the size is above the last message, then refrain from pushing the send queue into the websocket. Essentially, only do the websocket.send when there is nothing buffered. This can be checked with a simple timer running at designated intervals.
After a bit more thought and poking around, I realize that Angular 2's own HTTP module also returns cold observables. For example, http.get returns a cold observable, and requires a subscription in order to fire.
So, it may not be unreasonable for your send to return a cold observable -- at least it is consistent with the rest of Angular 2. The downside is, though, that HTTP calls are usually heavily minimized to reduce round-trips, so it is ok to incur the additional overhead of a subscription for each all. WebSockets are supposed to send huge amounts of information frequently back-and-forth; the extra subscription overheads may be excessive.
So, I suggest you keep send as it is, but then add a sendRaw which just sends without return an observable.
sendRaw can be very simple:
sendRaw(data) {
if (this.getReadyState() !== this.readyStateConstants.OPEN
&& this.getReadyState() !== this.readyStateConstants.CONNECTING) {
this.connect();
}
this.sendQueue.push({ message: data });
this.fireQueue();
};
@schungx I read your proposal, I think the send method is well in being a cold observable. What is the deal with send an object without subcribed?
Also reviewing the send method, I think this need to throw an error instead of observer.next('Socket connection has been closed');
@jmparra, WebSocket.send does not return a handle. It is fire-and-forget AFAIK. There is no reason for the send to return anything to observe. This is in direct contrast to Http.send which does return values.
The only reason why you'd want to return an observable that doesn't actually return anything is probably to unify the signatures of the data-sending mechanisms, so you can abstract them away in the future, essentially making Http.send and WebSocket.send interchangeable.
Another reason is that maybe in the future the implementation can be modified such that it does not dump all the messages in the send queue to the WebSocket at once, but only one at a time. Keeping unsent messages in an internal queue away from the WebSocket helps with recovery in case of disconnection. This way, you can also track whether a message is actually sent (sort of, because you can't know that; you have to assume that if it goes into the WebSocket it is sent). In that case, when it is sent, you can return a signal to the observable to any subscriber who cares whether the message has actually been sent.
Subscription to an observable adds complexity and overheads in the runtime, for absolutely no benefit (as I said, the actual send is fire-and-forget). It creates links and objects that are immediately thrown away. Now this is OK if it is not done very frequently, but WebSocket's are designed to send messages very frequently, much more frequently than XHR.
Therefore, in the case of heavy usage of the WebSocket, it is best to provide a low-overhead version of the send.
Regarding your second point, I disagree. The async nature of Observables mean that you should not be throwing an exception, but to return an error to the observable, exactly as it is currently done.
