From 27ed4db53763770c63919b44bc14fc238faf9c47 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 6 May 2017 00:27:44 +0200 Subject: [PATCH] feat: return channel number and channels from subscribe / unsubscribe calls --- index.js | 7 ++++--- test/auth.spec.js | 11 +++++++---- test/connection.spec.js | 1 + test/helper.js | 25 +++++++++++++++++++++++++ test/pubsub.spec.js | 34 ++++++++++++++-------------------- 5 files changed, 51 insertions(+), 27 deletions(-) diff --git a/index.js b/index.js index 52a29131cd..27a3af3627 100644 --- a/index.js +++ b/index.js @@ -112,6 +112,7 @@ function RedisClient (options, stream) { this.buffers = options.returnBuffers || options.detectBuffers; this.options = options; this.reply = 'ON'; // Returning replies is the default + this.subscribeChannels = []; // Init parser this.replyParser = createParser(this); this.createStream(); @@ -605,6 +606,7 @@ function subscribeUnsubscribe (self, reply, type) { type = type === 'unsubscribe' ? 'subscribe' : 'psubscribe'; // Make types consistent delete self.subscriptionSet[type + '_' + channel]; } + self.subscribeChannels.push(channel); } if (commandObj.args.length === 1 || self.subCommandsLeft === 1 || commandObj.args.length === 0 && (count === 0 || channel === null)) { @@ -623,10 +625,9 @@ function subscribeUnsubscribe (self, reply, type) { } self.commandQueue.shift(); if (typeof commandObj.callback === 'function') { - // TODO: The current return value is pretty useless. - // Evaluate to change this in v.3 to return all subscribed / unsubscribed channels in an array including the number of channels subscribed too - commandObj.callback(null, channel); + commandObj.callback(null, [count, self.subscribeChannels]); } + self.subscribeChannels = []; self.subCommandsLeft = 0; } else { if (self.subCommandsLeft !== 0) { diff --git a/test/auth.spec.js b/test/auth.spec.js index c3b5ea9b99..e54e7f3930 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -321,14 +321,17 @@ describe('client authentication', function () { assert.strictEqual(client.serverInfo.sync_full, '0'); }) .get('foo', helper.isString('bar')) - .subscribe(['foo', 'bar']) + .subscribe(['foo', 'bar', 'foo'], helper.isUnSubscribe(2, ['foo', 'bar', 'foo'])) .unsubscribe('foo') - .SUBSCRIBE('/foo', helper.isString('/foo')) + .SUBSCRIBE('/foo', helper.isUnSubscribe(2, '/foo')) .psubscribe('*') - .quit(helper.isString('OK')) // this might be interesting + .quit(helper.isString('OK')) .exec(function (err, res) { res[4] = res[4].substr(0, 9); - assert.deepEqual(res, ['OK', 'OK', 'OK', 'OK', '# Stats\r\n', 'bar', 'bar', 'foo', '/foo', '*', 'OK']); + assert.deepStrictEqual( + res, + ['OK', 'OK', 'OK', 'OK', '# Stats\r\n', 'bar', [2, ['foo', 'bar', 'foo']], [1, ['foo']], [2, ['/foo']], [3, ['*']], 'OK'] + ); end(); }); }); diff --git a/test/connection.spec.js b/test/connection.spec.js index 2e071b8774..2f34a56f5f 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -309,6 +309,7 @@ describe('connection tests', function () { assert.strictEqual(client.stream.listeners('timeout').length, 0); done(); }); + client.end(true); }); it('clears the socket timeout after a connection has been established', function (done) { diff --git a/test/helper.js b/test/helper.js index a4efc4cdd9..492b59a2ee 100644 --- a/test/helper.js +++ b/test/helper.js @@ -130,6 +130,31 @@ module.exports = { }; } }, + isUnSubscribe: function (count, channels, done) { + if (typeof count !== 'number') { + done = channels; + channels = count; + count = undefined; + } + if (typeof channels === 'function') { + done = count; + count = undefined; + } + if (typeof channels === 'string') { + channels = [channels]; + } + var len = channels.length; + return function (err, results) { + assert.strictEqual(err, null, 'expected an array, got: ' + err); + assert.strictEqual(Array.isArray(results), true, results); + assert.strictEqual(Array.isArray(results[1]), true, results); + assert.strictEqual(results[1].length, len, results); + assert.strictEqual(typeof results[0], 'number', results); + if (count) assert.strictEqual(count, results[0], results); + if (done) done(); + }; + + }, match: function (pattern, done) { return function (err, results) { assert.strictEqual(err, null, 'expected ' + pattern.toString() + ', got error: ' + err); diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index e800e88aad..41b334a20b 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -187,7 +187,7 @@ describe('publish/subscribe', function () { }); it('handles SUB UNSUB MSG SUB 2', function (done) { - sub.psubscribe('abc*', helper.isString('abc*')); + sub.psubscribe('abc*', helper.isUnSubscribe(1, 'abc*')); sub.subscribe('xyz'); sub.unsubscribe('xyz'); pub.publish('abcd', 'something'); @@ -256,7 +256,7 @@ describe('publish/subscribe', function () { }); sub.set('foo', 'bar'); sub.unsubscribe(function (err, res) { - assert.strictEqual(res, null); + assert.deepStrictEqual(res, [0, []]); }); sub.del('foo', done); }); @@ -427,7 +427,7 @@ describe('publish/subscribe', function () { }); sub.punsubscribe(function (err, res) { assert(!err); - assert.strictEqual(res, 'bla'); + assert.deepStrictEqual(res, [0, ['prefix:3', 'prefix:2', '5', 'test:a', 'bla']]); assert(all); all = false; // Make sure the callback is actually after the emit end(); @@ -468,19 +468,13 @@ describe('publish/subscribe', function () { }); }); - it('does not complain when unsubscribe is called and there are no subscriptions', function (done) { - sub.unsubscribe(function (err, res) { - assert.strictEqual(err, null); - assert.strictEqual(res, null); - done(); - }); + it('sub executes callback when unsubscribe is called and there are no subscriptions', function (done) { + sub.unsubscribe(helper.isUnSubscribe(0, [], done)); }); - it('executes callback when unsubscribe is called and there are no subscriptions', function (done) { - pub.unsubscribe(function (err, results) { - assert.strictEqual(null, results); - done(err); - }); + it('pub executes callback when unsubscribe is called and there are no subscriptions', function (done) { + pub.unsubscribe(helper.isUnSubscribe(0, [])); + pub.get('foo', done); }); }); @@ -491,7 +485,7 @@ describe('publish/subscribe', function () { }); sub.subscribe('/foo', function () { sub2.on('ready', function () { - sub2.batch().psubscribe('*', helper.isString('*')).exec(); + sub2.batch().psubscribe('*', helper.isUnSubscribe(1, '*')).exec(); sub2.subscribe('/foo'); sub2.on('pmessage', function (pattern, channel, message) { assert.strictEqual(pattern.inspect(), new Buffer('*').inspect()); @@ -500,7 +494,7 @@ describe('publish/subscribe', function () { sub2.quit(done); }); pub.pubsub('numsub', '/foo', function (err, res) { - assert.deepEqual(res, ['/foo', 2]); + assert.deepStrictEqual(res, ['/foo', 2]); }); // sub2 is counted twice as it subscribed with psubscribe and subscribe pub.publish('/foo', 'hello world', helper.isNumber(3)); @@ -527,8 +521,8 @@ describe('publish/subscribe', function () { batch.psubscribe('*'); batch.subscribe('/foo'); batch.unsubscribe('/foo'); - batch.unsubscribe(helper.isNull()); - batch.subscribe(['/foo'], helper.isString('/foo')); + batch.unsubscribe(helper.isUnSubscribe(1, [])); + batch.subscribe(['/foo'], helper.isUnSubscribe(2, '/foo')); batch.exec(function () { pub.pubsub('numsub', '/foo', function (err, res) { // There's one subscriber to this channel @@ -569,7 +563,7 @@ describe('publish/subscribe', function () { }); it('executes callback when punsubscribe is called and there are no subscriptions', function (done) { - pub.batch().punsubscribe(helper.isNull()).exec(done); + pub.batch().punsubscribe(helper.isUnSubscribe(0, [])).exec(done); }); }); @@ -654,7 +648,7 @@ describe('publish/subscribe', function () { .client('KILL', ['type', 'pubsub'], function () {}) .unsubscribe() .psubscribe(['pattern:*']) - .punsubscribe('unkown*') + .punsubscribe('unknown*') .punsubscribe(['pattern:*']) .exec(function (err, res) { sub.client('kill', ['type', 'pubsub']);