joewalnes / reconnecting-websocket

A small decorator for the JavaScript WebSocket API that automatically reconnects
MIT License
4.21k stars 968 forks source link

Onconnecting can't be overridden quick enough #30

Open Jakobud opened 9 years ago

Jakobud commented 9 years ago

I have this:

socket = new ReconnectingWebSocket("wss://push.planetside2.com/streaming?service-id=s:ps2maps");
socket.debug = true;
socket.onconnecting = function(e) {
  console.log("Websocket connecting........", e);
  return true;
};

I never see my message even though I get a successful connection. The problem is that the websocket tries to make a connection before the event handler can be overridden, so it never registers. Later, after a connection is made, I can manually (in dev tools) run:

socket.onconnecting() and now I see my message.

How would you solve this problem? One of the problems I've always seen about websockets is that they attempt to make a connection upon creation, which I think is a terrible idea. Its impossible to create a websocket without connecting immediately.

I think that is where your library could be improved. This is what I think needs to happen:

  1. Provide an option to create the reconnecting websocket object without actually making a connection. (ie, without creating a Websocket object). This would be something in the constructor, like:
var socket = new ReconnectingWebSocket("http://whatever.com", null, false);

The 3rd optional parameter would be a boolean (open when creating or not) that would default to true. That way backwards compatibility is maintained.

  1. Next, provide a manual .open() method that is used to manually make the connection. I made a pull request for this feature recently but it looks like the project code base has significantly changed since then. No big deal, its an easy one for me to rewrite. I know there is a refresh() method already, but if you manually close(1000), the socket connection for whatever reason, then refresh won't do anything. If there is a close() there needs to be an open().

These two options would allow you to create a reconnecting websocket without creating a websocket. Then you can define all your event handlers. Then you can manually connect. Then everything works great.

The HTML5 WebSocket API is not perfect, and this is a great opportunity for this library to fix some of the downfalls.

I could put together a pull request for this if you are interested in seeing it in action.

joewalnes commented 9 years ago

I see the problem.

Re: your initial problem of missing the original onconnecting event, my preferred fix will be to ensure that the event is not fired immediately in the constructor, but instead fired async (setTimeout(0)).

Re: your suggestion about explicit open()/close() operations. Yes I like this too. My only slight change your to proposal is to pass named options to the constructor to make it more obvious in the code and to prepare for other options in the future. e.g. new ReconnectingWebSocket('ws://...', null, {automaticOpen:false}). I'm open to suggestions for a better name for that parameter.

If you want to take a crack at either of these, please do them in separate pull requests.

Thanks

Jakobud commented 9 years ago

Good call on passing options. That's easy to do.

Can you elaborate on the first part? Do you mean that if you use a setTimeout with zero delay, that gives the script enough time to establish the onconnecting handler before the connection occurs?

Yeah I will put together a couple pull requests for you on this.

joewalnes commented 9 years ago

Yeah, if you replace the dispatchEvent(...) piece with setTimeout(function() { dispatchEvent(...) }, 0), it will queue the command to run async on the next iteration of the command loop, which will be after you've assigned the onconnecting handler.