search

oortcloud / node-ddp-client

A callback style DDP (Meteor's Distributed Data Protocol) node client.
Other
263 stars 80 forks source link

Correctly update internal connection status #86

Open heysailor opened 8 years ago

heysailor commented 8 years ago

The function argument to client.connect() is meant to be optional.

However, unless you pass a function argument, the internal booleans _isConnecting and _isReconnecting never get updated.

This PR correctly updates the booleans to false when a connected message is received from the server.

vsivsi commented 7 years ago

@heysailor

Hi, sorry for the long delay in looking at this. I'm trying to clean-up the old PRs that nobody has gotten around to in preparation for doing a release...

I have a question. It seems that with your proposed change, the assignments at these lines are now redundant and should be removed. Correct? https://github.com/oortcloud/node-ddp-client/blob/master/lib/ddp-client.js#L318-L319

But more concerningly, with that, there appears to be a change in semantics that I don't fully understand the implications of. See: https://github.com/oortcloud/node-ddp-client/pull/86/files#r90728686

Perhaps the correct way to address the issue you have identified here is to always register listeners for the 'connected' and 'failed' events, and move the conditional on the presence of the optional callback into each listener function.

Something like:

// if (connected) {
    self.addListener("connected", function() {
      self._clearReconnectTimeout();
      if (connected) {  // Condition moved here
         connected(undefined, self._isReconnecting);
      }
      self._isConnecting = false;
      self._isReconnecting = false;
    });
    self.addListener("failed", function(error) {
      self._isConnecting = false;
      self._connectionFailed = true;
      if (connected) {  // Condition moved here
         connected(error, self._isReconnecting);
      }
    });
//}

Does that make sense to you?