Add callbacks when message received, error occurs or connection closed

[sarfata]

I was not satisfied with the first solution and having multiple callbacks passed to connect() so I looked at ws and how they dealt with it. I have adopted the same pattern.

So I use events.EventEmitter and inherit from it. And then have different messages: on('connected'), on('message'), on('close') and on('error'). I believe this solution to be much better. I hope you will agree.

Cheers!

[tmeasday]

I agree this is a better approach for messages and errors[1].

I'm not sure about .on('connect') though. Is this to deal with temporary connection problems?

It's just that I forked this from Alan's code base because I wasn't really a fan of the event style where events were disassociated from the action that triggered them. e.g. I prefer:

  ddpclient.connect(function() {
    // set up initial subscriptions and whatnot
  });

To

  ddpclient.connect();

  ddpclient.on('connect', function() {..});

It's less of a big deal for connection (but then again if we want a 'reconnect' event (which might make sense), we'd want to do different things when that happened to what we'd do on the original connection right?).

What do you think?

[1] Although how do errors happen? Aren't they usually errors in response to a specific action that you've taken (e.g. a method call?)

[sarfata]

On errors: Those are socket errors: the errors that happen when the connection is closed or the server dies. For example when you save a file on the server and meteor reparses everything: it closes/reopens every connections. Without the error handler the connection dies and you have to restart the client.

I understand your concern about disassociating events from what triggered them. I did this because I think a lot of people will want to do something on connect, most likely subscribe to some collections. When they are disconnected, they will probably want to do the same thing again (to get the same subscriptions running).

So if we were to rewrite the example.js with the automatic reconnection on error, you would have to repeat the connect callback (at least) twice - we would get something like this:

ddpclient.connect(function() {
  ddpclient.subscribe('posts', [], function() {
    console.log('posts complete:');
    console.log(ddpclient.collections.posts);
  });
});

ddpclient.on('close', function(code, message) {
    console.log("Close: [%s] %s", code, message);
    // Automatically reconnect
    ddpclient.connect(function() {
      /* Here - We need to repeat the connect handler */
      console.log('posts complete:');
      console.log(ddpclient.collections.posts);
  });
});

So you would probably put the connect handler function in a variable and reference the same twice. I guess if you have to do that the on('connect') approach is just as good and has the advantage of being standard in the node world.

One thing that I still do not like so much is that on('connect') refers to the connected event on the DDP layer, while on('errors') and on('close') refer to the lower level websocket. Food for thought ...

[tmeasday]

Right. I think I get it. What do you think about the following:

1. If I call connect with a callback, I think it would reasonable to expect the library to re-run that callback if the underlying connection stops+restarts, given this is almost definitely the behaviour the developer wants.
   • Perhaps there should be a reconnected argument to the function so the developer can deal differently if they like. Then again you'd probably argue the same about a on('connect') message too.
2. Do you think that the socket events could perhaps be renamed to socket-error and socket-close?
   • considering there's a DDP level error message too, I think the first is needed as this is a DDP client after all.
   • is there a DDP close message? Or does the server just close the socket? If the second, we can probably just leave it as close.

Sorry about being such a pain about this, but I figure we may as well get it perfect.

[sarfata]

You are not being a pain at all - I appreciate the concern for quality!

I thought about your last message for a bit and I think that an even better option might be to have one callback in the connect method (like you suggested) and to automatically handle network errors and reconnect.

This way the developer would just call connect(function(){}) (we might add the reconnect argument) and would not have to worry about network errors: they would be taken care of automatically. We may add messages on('disconnected') and on('reconnected') to let him know when we are connected or not (and maybe a status() object like Meteor.status()).

Re-connecting is going to be the #1 usecase for on('errors') and on('close') so we might as well implement it in the library.

What do you say?

[tmeasday]

Right, so perhaps this proposal is a good one:

1. The library handles automatic reconnecting on errors/disconnects. Down the track we could add an option to turn this off but right now I don't think anyone wants any different behaviour.
2. The ddpclient emits 2 messages to help with debugging; message, reconnect.
3. Also the client makes the underlying websocket available as a property, so if you want to see underlying socket level events, you can do e.g. ddpclient.socket.on('error')

That way the socket level stuff is separated from the DDP level stuff. What do you think?

[sarfata]

Looks perfect. I will work on that sometimes this week.

thomas

On Thu, Feb 21, 2013 at 10:26 PM, Tom Coleman notifications@github.comwrote:

Right, so perhaps this proposal is a good one:

1.

The library handles automatic reconnecting on errors/disconnects. Down the track we could add an option to turn this off but right now I don't think anyone wants any different behaviour. 2.

The ddpclient emits 2 messages to help with debugging; message, reconnect. 3.

Also the client makes the underlying websocket available as a property, so if you want to see underlying socket level events, you can do e.g. ddpclient.socket.on('error')

That way the socket level stuff is separated from the DDP level stuff. What do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/oortcloud/node-ddp-client/pull/7#issuecomment-13916951.

[sarfata]

And by the way we might have to look at compatibility with 0.5.7 (in another pull request probably).

On Sat, Feb 23, 2013 at 12:27 PM, Thomas Sarlandie thomas@sarlandie.netwrote:

Looks perfect. I will work on that sometimes this week.

thomas

On Thu, Feb 21, 2013 at 10:26 PM, Tom Coleman notifications@github.comwrote:

Right, so perhaps this proposal is a good one:

1.

The library handles automatic reconnecting on errors/disconnects. Down the track we could add an option to turn this off but right now I don't think anyone wants any different behaviour. 2.

The ddpclient emits 2 messages to help with debugging; message, reconnect. 3.

Also the client makes the underlying websocket available as a property, so if you want to see underlying socket level events, you can do e.g. ddpclient.socket.on('error')

That way the socket level stuff is separated from the DDP level stuff. What do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/oortcloud/node-ddp-client/pull/7#issuecomment-13916951.

[tmeasday]

Yes, I think we might as well do one big release that hits 0.5.7.

I'll need to time it with an atmosphere/meteorite release as well. Not a big deal.

[sarfata]

So I did what we had planned but it turns out that because we wipe out the socket object every time before reconnecting, if the user had set some specific callback on ddp.socket, they would be lost. I could have saved them and restored them but I think it would have been just plain dirty (need to re-add only the user callbacks, not ours, etc). We could also try reusing the socket but I am not sure ws was designed to be used this way and wanted to avoid weird bugs after a re-connection.

So instead:

• We automatically reconnect (with two new opts to customize that if needed)
• We use the callback passed to the connect() function every time
• Two events: socket-error and socket-close allow the user to do specific things when disconnected (before we are reconnected)

One added advantage is that I can transmit the close(code, message) and error(error) parameters in the event, so that the user exactly knows what happened. This was not possible with just one reconnect event (because the parameters are different).

Have you looked at 0.5.7 already? Need help with that? I saw #8, do you have an idea of what is missing?

[tmeasday]

@sarfata -- looks great. Merging.

Regarding #8, I think it's just a matter of handing added/removed/changed rather than set/unset for collection messages. I'd love help with it. I'll give you git access.

Let me know if you want to release these changes separately or if we should just do one big 0.5.7 lump (I'd lean towards the second as the current code doesn't work with the latest Meteor which doesn't quite seem right).

[sarfata]

No rush for me for a release. I agree it would be best to also have the 0.5.7 working at the same time. No need for git access, I think I code a lot better when I have someone watching over my shoulder ;)

I will try to take a look asap.

thomas

On Sun, Mar 3, 2013 at 10:56 PM, Tom Coleman notifications@github.comwrote:

@sarfata https://github.com/sarfata -- looks great. Merging.

Regarding #8 https://github.com/oortcloud/node-ddp-client/issues/8, I think it's just a matter of handing added/removed/changed rather than set/unset for collection messages. I'd love help with it. I'll give you git access.

Let me know if you want to release these changes separately or if we should just do one big 0.5.7 lump (I'd lean towards the second as the current code doesn't work with the latest Meteor which doesn't quite seem right).

— Reply to this email directly or view it on GitHubhttps://github.com/oortcloud/node-ddp-client/pull/7#issuecomment-14357573 .

[tmeasday]

Oh, well I added you anyway I think.

Feel free to just code in branches if you want me to code review it before merging to master. Or send me PRs. Whatever works.

On Monday, 4 March 2013 at 10:00 PM, Thomas Sarlandie wrote:

No rush for me for a release. I agree it would be best to also have the
0.5.7 working at the same time. No need for git access, I think I code a
lot better when I have someone watching over my shoulder ;)

I will try to take a look asap.

thomas

On Sun, Mar 3, 2013 at 10:56 PM, Tom Coleman <notifications@github.com (mailto:notifications@github.com)>wrote:

@sarfata https://github.com/sarfata -- looks great. Merging.

Regarding #8 https://github.com/oortcloud/node-ddp-client/issues/8, I
think it's just a matter of handing added/removed/changed rather than
set/unset for collection messages. I'd love help with it. I'll give you
git access.

Let me know if you want to release these changes separately or if we
should just do one big 0.5.7 lump (I'd lean towards the second as the
current code doesn't work with the latest Meteor which doesn't quite seem
right).

—
Reply to this email directly or view it on GitHubhttps://github.com/oortcloud/node-ddp-client/pull/7#issuecomment-14357573
.

— Reply to this email directly or view it on GitHub (https://github.com/oortcloud/node-ddp-client/pull/7#issuecomment-14375236).