Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure we reconnect with the same options passed to start #347

Merged
merged 4 commits into from
May 17, 2017
Merged

Ensure we reconnect with the same options passed to start #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5126a4
Write a failing test that demonstrates the issue.
CharlieHess May 17, 2017
7b422a1
Fix whoœps in the README.
CharlieHess May 17, 2017
a77b366
Make the test green.
CharlieHess May 17, 2017
3968f9c
Update example to address #300.
CharlieHess May 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions README.md
View file
Open in desktop
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
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are a saint for fixing things like this 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i gotchu dawg

* @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
Open in desktop
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
Open in desktop
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