@slack/client - npm Package Compare versions

Comparing version 2.2.1 to 2.3.0-beta.1

CHANGELOG.md

@@ -0,1 +1,7 @@
+### v2.3.0 ()
+
+  * Caches messages on the RTM client, to improve handling in cases where message send fails
+  * Removes the handler for the websocket level `ping` handler (not the RTM API level ping handler)
+  * Refactors the logic for handling ws send responses to a single function
+
 ### v2.2.1 (2016-03-12)
@@ -2,0 +8,0 @@

lib/clients/rtm/client.js

@@ -15,2 +15,3 @@ /**
 var noop = require('lodash').noop;
+var keys = require('lodash').keys;
 
@@ -82,2 +83,8 @@ var RTM_API_EVENTS = require('../events/rtm').EVENTS;
   /**
+   * @type {{}}
+   * @private
+   */
+  this._pendingMessages = {};
+
+  /**
    *
@@ -89,3 +96,2 @@ * @type {{}}
 
-
   /**
@@ -104,2 +110,5 @@ *
   this.RECONNECTION_BACKOFF = clientOpts.reconnectionBackoff || 3000;
+
+  // NOTE: see the "Ping and Pong" section of https://api.slack.com/rtm
+  //       these are to do with the RTM API level connection and not the underlying ws connection.
   this.MAX_PONG_INTERVAL = clientOpts.maxPongInterval || 10000;
@@ -305,3 +314,2 @@ this.WS_PING_INTERVAL = clientOpts.wsPingInterval || 5000;
   this.ws.on('close', bind(this.handleWsClose, this));
-  this.ws.on('ping', bind(this.handleWsPing, this));
 };
@@ -350,4 +358,2 @@
   if (this.connected) {
-    this.send({ type: 'ping' }, noop);
-
     // If the last pong was more than MAX_PONG_INTERVAL, force a reconnect
@@ -357,2 +363,4 @@ pongInterval = Date.now() - this._lastPong;
       this.reconnect();
+    } else {
+      this.send({ type: 'ping' }, noop);
     }
@@ -437,15 +445,8 @@ }
 
-  // This means that the message is an ack of a message sent via the websocket from the client.
-  // The expected structure is something like:
-  // {
-  //   "ok": true,
-  //   "reply_to": 1,
-  //   "ts": "1355517523.000005",
-  //   "text": "Hello world"
-  // }
-  // ... in the case of success.
-  //
-  // See the Sending messages section of https://api.slack.com/rtm for details.
   if (replyTo) {
-    this._handleMsgReply(replyTo, message, messageType);
+    if (!messageType) {
+      this._handleMessageAck(replyTo, message);
+    } else {
+      this._handleMostRecentMsgReply(replyTo, message);
+    }
   } else {
@@ -462,67 +463,104 @@ // Non reply_to messages should *always* have a type
 /**
- *
+ * Handler for the remote server's response to a message being sent on the websocket.
  * @param replyTo
  * @param message
- * @param messageType
- * @private
  */
-RTMClient.prototype._handleMsgReply = function _handleMsgReply(replyTo, message, messageType) {
+RTMClient.prototype._handleMessageAck = function handleMessageAck(replyTo, message) {
   var msg;
-  var channel;
+  var channelId;
 
-  if (!messageType) {
-    if (hasKey(this._msgResponseHandlers, replyTo)) {
-      if (!message.ok) {
-        this._handleMsgResponse(replyTo, new SlackRTMSendError(message.error.msg, message), null);
-      } else {
-        channel = this._msgChannelLookup[replyTo];
+  // This means that the message is an ack of a message sent via the websocket from the client.
+  // The expected structure is something like:
+  // {
+  //   "ok": true,
+  //   "reply_to": 1,
+  //   "ts": "1355517523.000005",
+  //   "text": "Hello world"
+  // }
+  // ... in the case of success.
+  //
+  // See the Sending messages section of https://api.slack.com/rtm for details.
 
-        // Make a synthetic message, rather than passing back the raw ack response
-        msg = {
-          type: 'message',
-          channel: channel,
-          user: this.activeUserId,
-          text: message.text,
-          ts: message.ts
-        };
+  if (hasKey(this._msgResponseHandlers, replyTo)) {
+    if (!message.ok) {
+      this._handleMsgResponse(replyTo, new SlackRTMSendError(message.error.msg, message), null);
+    } else {
+      channelId = this._msgChannelLookup[replyTo];
 
-        if (this.dataStore) {
-          // TODO(leah): Update the relevant channel history with this message
-          // this.dataStore.getChannelById(channel);
-        }
+      // Make a synthetic message, rather than passing back the raw ack response
+      msg = {
+        type: 'message',
+        channel: channelId,
+        user: this.activeUserId,
+        text: message.text,
+        ts: message.ts
+      };
 
-        this._handleMsgResponse(replyTo, null, msg);
+      if (this.dataStore) {
+        // NOTE: this will treat the message response as canonical and assumes that no temporary
+        //       message has been put in place.
+        this.dataStore.handleRtmMessage(this.activeUserId, this.activeTeamId, 'message', msg);
       }
-    } else {
-      // This should be impossible. If the message is received with no message type, it's a response
-      // to a message sent by this client, so the absence of a handler for it should never happen.
-      this.logger.error('message received with unknown reply_to: ' + message);
+
+      this._handleMsgResponse(replyTo, null, msg);
     }
   } else {
-    // The server will send the most recent reply message to the client when it first connects,
-    // so will receive a message that looks like:
-    //
-    // {
-    //   "reply_to": 848,
-    //   "type": "message",
-    //   "channel": "C0CHZA86Q",
-    //   "user": "U0CJ5PC7L",
-    //   "text": "meh-mah",
-    //   "ts": "1457327357.000011"
-    // }
-    //
-    // This should generally only happen in the case of disconnections. If that happens, the last
-    // message should be handled gracefully.
+    // This should be impossible. If the message is received with no message type, it's a response
+    // to a message sent by this client, so the absence of a handler for it should never happen.
+    this.logger.error('message received with unknown reply_to: ' + message);
+  }
+};
 
-    // TODO(leah): The client should probably also enqueue messages locally so that they can resent
-    // in the event of disconnection
 
-    if (this._msgResponseHandlers[replyTo]) {
-      msg = cloneDeep(message);
-      delete msg.reply_to;
-      this._handleMsgResponse(replyTo, null, msg);
+/**
+ * Handler for the server
+ * @param {} replyTo
+ * @param {{}} message
+ * @private
+ */
+RTMClient.prototype._handleMostRecentMsgReply = function _handleMostRecentMsgReply(
+  replyTo, message) {
+  var msg;
+  var pendingMessageIds;
+  var failedToSendErr;
+
+  // The server will send the most recent reply message to the client when it first connects,
+  // so will receive a message that looks like:
+  //
+  // {
+  //   "reply_to": 848,
+  //   "type": "message",
+  //   "channel": "C0CHZA86Q",
+  //   "user": "U0CJ5PC7L",
+  //   "text": "meh-mah",
+  //   "ts": "1457327357.000011"
+  // }
+  //
+  // This should generally only happen in the case of disconnections. If that happens, the last
+  // message should be handled gracefully.
+
+  if (this._msgResponseHandlers[replyTo]) {
+    msg = cloneDeep(message);
+    delete msg.reply_to;
+    this._handleMsgResponse(replyTo, null, msg);
+  } else {
+    // The only time this should happen is when a client first connects
+    this.logger('info', 'message received on reconnect with no registered callback:\n' + message);
+  }
+
+  pendingMessageIds = keys(this._pendingMessages);
+  forEach(pendingMessageIds, function handlePendingMessage(messageId) {
+    // If the message id is less than the most recent reply to id, assume that the message has
+    // been processed. If it's greater than the most recent reply to, then it's likely that the
+    // message didn't get sent to the remote server. This should almost never happen, so for now if
+    // it does, call the pending callback with a custom error
+
+    if (messageId > replyTo) {
+      failedToSendErr = new SlackRTMSendError(
+        'message not sent due to connection trouble', this._pendingMessages[messageId]);
+      this._handleMsgResponse(messageId, failedToSendErr, null);
     } else {
-      // TODO(leah): Figure out what to do here
+      this._clearMessageState(messageId);
     }
-  }
+  }, this);
 };
@@ -560,14 +598,3 @@
 /**
- * Handler for the websocket ping event.
- * This pongs back to the server to let it know the client is still active.
- */
-RTMClient.prototype.handleWsPing = function handleWsPing() {
-  if (this.ws.pong) {
-    this.ws.pong();
-  }
-};
-
-
-/**
- * Handles the server's pong message, updating the lastPong time on the client.
+ * Handles the RTM API's pong message, updating the lastPong time on the client.
  * @param {Object} message
@@ -657,2 +684,6 @@ */
   var msgId = this.nextMessageId();
+  var _handleWsSendResponse = bind(this._handleWsSendResponse, this, msgId);
+  // NOTE: this is done before we clone and add the id property to the message, so that if the
+  //       message is retried it'll definitely get a new id
+  this._pendingMessages[msgId] = message;
 
@@ -667,15 +698,7 @@ var wsMsg = cloneDeep(message);
     _this._registerMsgHandler(msgId, wsMsg, optCb);
-    this.ws.send(jsonMessage, undefined, function handleWsMsgResponseCb(wsRespErr) {
-      if (!isUndefined(wsRespErr)) {
-        _this._handleMsgResponse(msgId, wsRespErr);
-      }
-    });
+    this.ws.send(jsonMessage, undefined, _handleWsSendResponse);
   } else {
     ret = new Promise(function wsSendPromiseInner(fulfill, reject) {
       _this._registerMsgHandler(msgId, wsMsg, { fulfill: fulfill, reject: reject });
-      _this.ws.send(jsonMessage, undefined, function handleWsMsgResponseCb(wsRespErr) {
-        if (!isUndefined(wsRespErr)) {
-          _this._handleMsgResponse(msgId, wsRespErr, null);
-        }
-      });
+      _this.ws.send(jsonMessage, undefined, _handleWsSendResponse);
     });
@@ -689,2 +712,19 @@ }
 /**
+ *
+ * @param msgId
+ * @param wsRespErr
+ * @private
+ */
+RTMClient.prototype._handleWsSendResponse = function _handleWsSendResponse(msgId, wsRespErr) {
+  if (wsRespErr) {
+    this._handleMsgResponse(msgId, wsRespErr, null);
+  } else {
+    // TODO(leah): This should probably clear the enqueued message object from _pendingMessages, as
+    //             the server has the data at this point. Figure out how this will interact with
+    //             the logic in _handleMostRecentMsgReply
+  }
+};
+
+
+/**
  * Registers a handler, either a function or {fulfill: fn, reject: fn}, for a message id.
@@ -738,4 +778,10 @@ * @param {number} msgId The id of the message to handle.
 
+  this._clearMessageState(msgId);
+};
+
+
+RTMClient.prototype._clearMessageState = function _clearMessageState(msgId) {
   delete this._msgChannelLookup[msgId];
   delete this._msgResponseHandlers[msgId];
+  delete this._pendingMessages[msgId];
 };
@@ -742,0 +788,0 @@

package.json

 {
   "name": "@slack/client",
-  "version": "2.2.1",
+  "version": "2.3.0-beta.1",
   "description": "A library for creating a Slack client",
@@ -5,0 +5,0 @@ "main": "./index",

New alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

No v1

Quality

Package is not semver >=1. This means it is not stable and does not support ^ ranges.

Found 1 instance in 1 package

Fixed alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

Improved metrics

Total package byte prevSize

221179

increased by0.97%

Lines of code

5956

increased by0.66%

Worsened metrics

Number of low quality alerts

2

increased by100%

No dependency changes