-
-
Notifications
You must be signed in to change notification settings - Fork 233
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.
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
presence module for aedes #69
Conversation
t.ok(client, 'client must be passed to published method') | ||
|
||
cb() | ||
if (packet.topic.split('/').pop() !== 'clientDisconnect') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case was failing because the client object was made null after Aedes publish. Let me know if you have a better way of going about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. but we should put a note here, and maybe in the docs too. For server publishes the client is always null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its already mentioned in the published method block in the readme. I'll add it here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Some nits to be fixed
@@ -223,6 +223,7 @@ var publishFuncsQoS = [ | |||
callPublished | |||
] | |||
Aedes.prototype.publish = function (packet, client, done) { | |||
//console.log(packet, client, done) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove this?
this.publish({ | ||
topic: '$SYS/' + this.id + '/new/clientDisconnect', | ||
payload: new Buffer(client.id, 'utf8') | ||
}, noop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be named differently. Would you mind checking what's the situation in Mosquitto and Mosca?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mosquitto doesn't provide this infomation. It provides the number of connected/disconnected clients. Mosca uses $SYS/+/new/clients & $SYS/+/disconnect/clients for connections & disconnections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the same as mosca then :)
t.ok(client, 'client must be passed to published method') | ||
|
||
cb() | ||
if (packet.topic.split('/').pop() !== 'clientDisconnect') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. but we should put a note here, and maybe in the docs too. For server publishes the client is always null.
1 similar comment
Would this require a change in the Readme ? |
Yes, please. |
Done. |
Good Job. |
An online/offline feature for aedes