Correctly update internal connection status #86
Conversation
Unless you pass a function argument to client.connect(), the internal booleans _isConnecting and _isReconnecting never get updated. The function argument is meant to be optional.
| @@ -146,6 +146,8 @@ DDPClient.prototype._message = function(data) { | |||
|
|
|||
| } else if (data.msg === "connected") { | |||
| self.session = data.session; | |||
| self._isConnecting = false; | |||
| self._isReconnecting = false; | |||
| self.emit("connected"); | |||
There was a problem hiding this comment.
This emit triggers a call to this listener (when registered by providing a connect function):
https://github.com/oortcloud/node-ddp-client/blob/master/lib/ddp-client.js#L314-L320
In the current code, that listener function calls connect (passing self._isReconnecting as a parm) before clearing self._isReconnecting in the subsequent lines. With this change, the value of self._isReconnecting will be cleared prior to that call being made, potentially changing the behavior of a registered connect function.
|
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? 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 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? |
The function argument to
client.connect()is meant to be optional.However, unless you pass a function argument, the internal
booleans
_isConnectingand_isReconnectingnever get updated.This PR correctly updates the booleans to
falsewhen aconnectedmessage is received from the server.