Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

add message to publish callback #527

Merged
merged 3 commits into from Jul 24, 2016
Merged

add message to publish callback #527

merged 3 commits into from Jul 24, 2016

Conversation

spali
Copy link
Contributor

@spali spali commented Jul 23, 2016

allows the callback to get the generated message id or any other content.
i.e. usefully for filtering own published message from the published hook.

against the contribution guidelines, could not run tests for this commit. Currently have some problems with other dependencies. Hope this small change didn't break anything. At least could find code that should break with this.

@@ -380,7 +380,7 @@ Server.prototype.publish = function publish(packet, client, callback) {
logger.debug({ packet: newPacket }, "published packet");
}
that.emit("published", newPacket, client);
callback();
callback(newPacket);
Copy link
Collaborator

@mcollina mcollina Jul 23, 2016

Choose a reason for hiding this comment

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

can you move this as the second argument? The first one is for the error.

@mcollina
Copy link
Collaborator

This is missing a unit test.

@spali
Copy link
Contributor Author

spali commented Jul 23, 2016

Could get test to run on my client.
I'm really new to this, would be my first test :)

I would put it in tests/server.js after the test "should support subscribing via server.subscribe":
Hope this makes any sence as test

  it("should provide packet in callback", function(done) {
    var messageId;

    this.instance.once("published", function(packet) {
      messageId = packet.messageId;
    });

    this.instance.publish({
      topic: "hello",
      payload: "some data"
    }, function(error, packet) {
      expect(packet.topic).to.be.equal("hello");
      expect(packet.payload.toString().toString()).to.be.equal("some data");
      expect(packet.messageId.toString()).to.equal(messageId);
      done();
    });
  });

please let me know if I should push this to the PR.

@mcollina
Copy link
Collaborator

I would cal it "should provide packet in publish callback". Anyway, yes,
push!

Il giorno sab 23 lug 2016 alle 20:06 Thomas Spalinger <
notifications@github.com> ha scritto:

Could get test to run on my client.
I'm really new to this, would be my first test :)

I would put it in tests/server.js after the test "should support
subscribing via server.subscribe":
Hope this makes any sence as test

it("should provide packet in callback", function(done) {
var messageId;

this.instance.once("published", function(packet) {
  messageId = packet.messageId;
});

this.instance.publish({
  topic: "hello",
  payload: "some data"
}, function(error, packet) {
  expect(packet.topic).to.be.equal("hello");
  expect(packet.payload.toString().toString()).to.be.equal("some data");
  expect(packet.messageId.toString()).to.equal(messageId);
  done();
});

});

please let me know if I should push this to the PR.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#527 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADL4zaGHJisKonaLZ6a8fn-YLJACx3lks5qYlgngaJpZM4JTWSO
.

@spali
Copy link
Contributor Author

spali commented Jul 23, 2016

Renamed the test in the commit as you supposed.
My test seems to work, but mongo was not available in the travis build. Locally all tests passed with node 6.3. So I assume this PR can be merged.

@mcollina mcollina merged commit 4c46e75 into moscajs:master Jul 24, 2016
@mcollina
Copy link
Collaborator

Thanks!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants