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

Pass options to backend #26

Merged
merged 2 commits into from
May 24, 2013
Merged

Pass options to backend #26

merged 2 commits into from
May 24, 2013

Conversation

davedoesdev
Copy link
Contributor

When publishing message, pass qos, clientId and messageId to ascoltatore.
When forwarding message, pass on qos and messageId to client.
When subscribing, pass qos to asoltatore.
Close ascoltatore when server is closed.
Tests for passing on qos and messageId.
Update ascoltatori dependency to 0.5.0.

…ore.

When forwarding message, pass on qos and messageId to client.
When subscribing, pass qos to asoltatore.
Close ascoltatore when server is closed.
Tests for passing on qos and messageId.
@mcollina
Copy link
Collaborator

Cool! I've just noticed there is a problem on Ascoltatori.
At this line https://github.com/mcollina/ascoltatori/blob/master/lib/mqtt_ascoltatore.js#L90 we are checking if there is already a subscription for that topic. However, we are not checking QOS levels, so we may have an old qos 0 subscription masquerading a 'new' qos 1.
As these are unrelated, do you need this in Mosca to add a couple of tests for Ascoltatori, right?

@davedoesdev
Copy link
Contributor Author

Good catch. Yes, adding this pull request is independent and then I can write a test in Ascoltatori to make two subscriptions and check we get messages with the appropriate QoS.
I'll also write a test mirroring the one that's already in memory_ascoltatore_spec.js ("should publish with options").

@davedoesdev
Copy link
Contributor Author

Let me know when you get time to merge it and then I'll do those Ascoltatori tests

mcollina added a commit that referenced this pull request May 24, 2013
Pass QoS level to the backing MQTTAscoltatore in the tree scenario.
@mcollina mcollina merged commit 9fa4c7a into moscajs:master May 24, 2013
@mcollina
Copy link
Collaborator

Published to npm!

@mcollina
Copy link
Collaborator

Thanks again 🍻

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.

2 participants