Skip to content

Commit

Permalink
Merge pull request #347 from CharlieHess/reconnect_fixes
Browse files Browse the repository at this point in the history 
Ensure we reconnect with the same options passed to start
  • Loading branch information
@aoberoi
aoberoi authored May 17, 2017
2 parents 6f46169 + 3968f9c commit 2b8ee33
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 43 deletions.
33 changes: 19 additions & 14 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

Read the [full documentation](https://slackapi.github.io/node-slack-sdk) for all the lovely details.

So you want to build a Slack app with Node.js? We've got you covered. {{ site.product_name }} is aimed at making
So you want to build a Slack app with Node.js? We've got you covered. `node-slack-sdk` is aimed at making
building Slack apps ridiculously easy. This module will help you build on all aspects of the Slack platform,
from dropping notifications in channels to fully interactive bots.

Expand All @@ -24,7 +24,7 @@ This library does not attempt to provide application level support, _e.g._ regex
conversation stream.

Most Slack apps are interested in posting messages into Slack channels, and generally working with our [Web API](https://api.slack.com/web). Read on
to learn how to use {{ site.product_name }} to accomplish these tasks. Bots, on the other hand, are a bit more complex,
to learn how to use `node-slack-sdk` to accomplish these tasks. Bots, on the other hand, are a bit more complex,
so we have them covered in [Building Bots](https://slackapi.github.io/node-slack-sdk/bots).

# Some Examples
Expand All @@ -47,11 +47,11 @@ var url = process.env.SLACK_WEBHOOK_URL || ''; //see section above on sensitive
var webhook = new IncomingWebhook(url);

webhook.send('Hello there', function(err, res) {
if (err) {
console.log('Error:', err);
} else {
console.log('Message sent: ', res);
}
if (err) {
console.log('Error:', err);
} else {
console.log('Message sent: ', res);
}
});
```

Expand All @@ -70,11 +70,11 @@ var token = process.env.SLACK_API_TOKEN || ''; //see section above on sensitive

var web = new WebClient(token);
web.chat.postMessage('C1232456', 'Hello there', function(err, res) {
if (err) {
console.log('Error:', err);
} else {
console.log('Message sent: ', res);
}
if (err) {
console.log('Error:', err);
} else {
console.log('Message sent: ', res);
}
});
```

Expand All @@ -93,8 +93,13 @@ var bot_token = process.env.SLACK_BOT_TOKEN || '';

var rtm = new RtmClient(bot_token);

// The client will emit an RTM.AUTHENTICATED event on successful connection, with the `rtm.start` payload if you want to cache it
rtm.on(CLIENT_EVENTS.RTM.AUTHENTICATED, function (rtmStartData) {
let channel;

// The client will emit an RTM.AUTHENTICATED event on successful connection, with the `rtm.start` payload
rtm.on(CLIENT_EVENTS.RTM.AUTHENTICATED, (rtmStartData) => {
for (const c of rtmStartData.channels) {
if (c.is_member && c.name ==='general') { channel = c.id }
}
console.log(`Logged in as ${rtmStartData.self.name} of team ${rtmStartData.team.name}, but not yet connected to a channel`);
});

Expand Down
30 changes: 22 additions & 8 deletions lib/clients/rtm/client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@ RTMClient.prototype.activeUserId = undefined;
RTMClient.prototype.activeTeamId = undefined;


/**
* @type {SlackDataStore}
*/
RTMClient.prototype.dataStore = undefined;


/**
* The timer repeatedly pinging the server to let it know the client is still alive.
* @type {?}
Expand All @@ -175,7 +181,7 @@ RTMClient.prototype._lastPong = 0;


/**
*
* A running count of socket connection attempts.
* @type {number}
* @private
*/
Expand All @@ -191,17 +197,19 @@ RTMClient.prototype._connecting = false;


/**
* Whether the server is currently re-connecting.
* @type {boolean}
* Options passed to `start`, for use when reconnecting.
* @type {object}
* @private
*/
RTMClient.prototype._reconnecting = false;
RTMClient.prototype._startOpts = null;


/**
* @type {SlackDataStore}
* Whether the server is currently reconnecting.
* @type {boolean}
* @private
*/
RTMClient.prototype.dataStore = undefined;
RTMClient.prototype._reconnecting = false;


/** @inheritDoc */
Expand Down Expand Up @@ -234,6 +242,7 @@ RTMClient.prototype.start = function start(opts) {
this.logger('verbose', 'attempting to connect via the RTM API');
this.emit(CLIENT_EVENTS.CONNECTING);
this._connecting = true;
this._startOpts = opts;

if (this._useRtmConnect) {
this._rtm.connect(bind(this._onStart, this));
Expand Down Expand Up @@ -350,7 +359,7 @@ RTMClient.prototype.disconnect = function disconnect(optErr, optCode) {


/**
*
* Attempts to reconnect to the websocket.
*/
RTMClient.prototype.reconnect = function reconnect() {
if (!this._reconnecting) {
Expand All @@ -367,7 +376,12 @@ RTMClient.prototype.reconnect = function reconnect() {
'unable to connect to Slack RTM API, failed after max reconnection attempts'
);
}
setTimeout(bind(this.start, this), this._connAttempts * this.RECONNECTION_BACKOFF);

// Ensure we use the same arguments to `start` when reconnecting
setTimeout(
bind(this.start, this, this._startOpts),
this._connAttempts * this.RECONNECTION_BACKOFF
);
}
};

Expand Down
10 changes: 6 additions & 4 deletions lib/clients/web/facets/rtm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ function RtmFacet(makeAPICall) {
* @see {@link https://api.slack.com/methods/rtm.start|rtm.start}
*
* @param {Object=} opts
* @param {?} opts.simple_latest - Return timestamp only for latest message object of each channel
* (improves performance).
* @param {?} opts.no_unreads - Skip unread counts for each channel (improves performance).
* @param {?} opts.mpim_aware - Returns MPIMs to the client in the API response.
* @param {Boolean} opts.simple_latest - Return timestamp only for latest message object of each
* channel (improves performance).
* @param {Boolean} opts.no_unreads - Skip unread counts for each channel (improves performance).
* @param {Boolean} opts.no_latest - Exclude latest timestamps for channels, groups, mpims, and
* ims. Automatically sets no_unreads to 1
* @param {Boolean} opts.mpim_aware - Returns MPIMs to the client in the API response.
* @param {function=} optCb Optional callback, if not using promises.
*/
RtmFacet.prototype.start = function start(opts, optCb) {
Expand Down
55 changes: 38 additions & 17 deletions test/clients/rtm/client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ var RTM_JSON = require('../../fixtures/rtm.start.json');
var RTM_CONNECT_JSON = require('../../fixtures/rtm.connect.json');
var RtmAPIClient = require('../../../lib/clients/rtm/client');
var MockWSServer = require('../../utils/mock-ws-server');

var WS_PORT_NUMBER = 5221;

describe('RTM API Client', function () {
var createRtmClient = function (opts) {
Expand All @@ -18,7 +18,7 @@ describe('RTM API Client', function () {

describe('reconnection logic', function () {

var setUpTest = function (wssPort, fakeSlackUrl, useConnect) {
var mockWebSocket = function (wssPort, fakeSlackUrl, useConnect) {
var rtmFixture = useConnect
? lodash.cloneDeep(RTM_CONNECT_JSON)
: lodash.cloneDeep(RTM_JSON);
Expand All @@ -33,27 +33,30 @@ describe('RTM API Client', function () {
return new MockWSServer({ port: wssPort });
};

var testReconnectionLogic = function (onFirstConn, onSecondConnFn, opts, wssPort, done) {
var testReconnectionLogic = function (onFirstConn, onSecondConnFn, done, opts, startOpts) {
var clonedOpts = lodash.cloneDeep(opts);
var rtm;
var rtmConnCount;
// Make a fake URL as otherwise the test cases run in parallel and exhaust the nock-ed endpoint with the customized ws:// url
var fakeSlackUrl = 'https://slack.com:' + wssPort + '/api';
var wss = setUpTest(wssPort, fakeSlackUrl);

// Make a fake URL as otherwise the test cases run in parallel and exhaust the nock-ed
// endpoint with the customized ws:// url
var fakeSlackUrl = 'https://slack.com:' + WS_PORT_NUMBER++ + '/api';
var wss = mockWebSocket(WS_PORT_NUMBER, fakeSlackUrl);

clonedOpts = clonedOpts || { reconnectionBackoff: 1 };
clonedOpts.slackAPIUrl = fakeSlackUrl;
rtm = createRtmClient(clonedOpts);

sinon.spy(rtm, 'reconnect');
rtm.start();
rtm.start = sinon.stub(rtm, 'start', rtm.start);
rtm.start(startOpts);

rtmConnCount = 0;
rtm.on(RTM_CLIENT_EVENTS.RTM_CONNECTION_OPENED, function () {
rtmConnCount++;
if (rtmConnCount === 1) {
onFirstConn(wss, rtm);
}

if (rtmConnCount === 2) {
} else if (rtmConnCount === 2) {
onSecondConnFn(rtm);
rtm.disconnect();
rtm = null;
Expand All @@ -74,7 +77,7 @@ describe('RTM API Client', function () {
reconnectionBackoff: 1
};

testReconnectionLogic(lodash.noop, onSecondConnFn, opts, 5221, done);
testReconnectionLogic(lodash.noop, onSecondConnFn, done, opts);
});

it('should reconnect when the websocket closes and auto-reconnect is true', function (done) {
Expand All @@ -86,7 +89,7 @@ describe('RTM API Client', function () {
expect(rtm.reconnect.calledOnce).to.equal(true);
};

testReconnectionLogic(onFirstConn, onSecondConnFn, { reconnectionBackoff: 1 }, 5222, done);
testReconnectionLogic(onFirstConn, onSecondConnFn, done);
});


Expand All @@ -106,7 +109,7 @@ describe('RTM API Client', function () {
expect(attemptingReconnectSpy.calledTwice).to.equal(true);
};

testReconnectionLogic(onFirstConn, onSecondConnFn, { reconnectionBackoff: 1 }, 5223, done);
testReconnectionLogic(onFirstConn, onSecondConnFn, done);
});


Expand All @@ -119,17 +122,35 @@ describe('RTM API Client', function () {
expect(rtm.reconnect.calledOnce).to.equal(true);
};

testReconnectionLogic(onFirstConn, onSecondConnFn, { reconnectionBackoff: 1 }, 5224, done);
testReconnectionLogic(onFirstConn, onSecondConnFn, done);
});


it('should pass the same start arguments when reconnecting', function (done) {
var startOpts = {
simple_latest: 1
};

var onFirstConn = function (wss) {
wss.closeClientConn();
};

var onSecondConnFn = function (rtm) {
expect(rtm.start.calledTwice).to.equal(true);
expect(rtm.start.getCall(0).args[0]).to.equal(startOpts);
expect(rtm.start.getCall(1).args[0]).to.equal(startOpts);
};

testReconnectionLogic(onFirstConn, onSecondConnFn, done, null, startOpts);
});


it('should support connecting to a socket via rtm.connect', function (done) {
var rtm;
var useRtmConnect = true;
var wssPort = 5225;
var fakeSlackUrl = 'https://slack.com:' + wssPort + '/api';
var fakeSlackUrl = 'https://slack.com:' + WS_PORT_NUMBER++ + '/api';

setUpTest(wssPort, fakeSlackUrl, useRtmConnect);
mockWebSocket(WS_PORT_NUMBER, fakeSlackUrl, useRtmConnect);
rtm = createRtmClient({
slackAPIUrl: fakeSlackUrl,
useRtmConnect: useRtmConnect
Expand Down

0 comments on commit 2b8ee33

Please sign in to comment.