-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(transport): add mqtt #4
Conversation
const data = { test: 'test' } | ||
|
||
await transport.subscribe('testing-channel', (payload) => { | ||
assert.deepEqual(payload, data) |
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 is invalid : If the subscribe
is never called then the assertion will never be executed and the test will create a false positive. We should use done
or the assertion plannings
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.
So, is the "message should be encoded and decoded correctly when using JSON encoder"
test from Redis also invalid? The test doesn't check assert.plan(1)
because there is no done()
call, and it uses only one transport, which can't receive its proper data.
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 one ?
https://github.com/boringnode/bus/blob/main/tests/drivers/redis_transport.spec.ts#L80
Yeah, definitely invalid too. This line will never be called https://github.com/boringnode/bus/blob/main/tests/drivers/redis_transport.spec.ts#L92
and this is a good example of what I was trying to explain. The test passes even though the assertion was never called
I'm really not familiar with MQTT. What are the most commonly used brokers? It would be cool to include test for those Also, please update the documentation in the README |
Hello @Julien-R44, |
The tests seem to pass. I do have one question, though. RabbitMQ is a broker that supports acknowledgments, right? So, doesn’t the worker need to acknowledge the job before it’s removed from the bus? |
I don't know very well RabbitMQ (I have never use it) but yeah it support acknowledgments like mqtt brokers. Yes good suggestion, I have forgot that but I can add the QoS option to the |
This bus package is designed as a fire-and-forget broker, so we're intentionally avoiding any acknowledgment logic. The question is: can we use RabbitMQ without acknowledgments, or will the message remain in the bus until it's acknowledged by a consumer? |
Ah ok my bad. |
Is this PR ready @MaximeMRF? Looks all good to me 👍 Any additional comments @RomainLanz, or can we merge this? |
@Julien-R44 hi before merge i have to provide the docs |
All questions are answered on my side! Do you plan to use it in your project @MaximeMRF? |
Hi @RomainLanz, Not atm but it can be a interesting alternative to Redis depending of the project. For example, I have a IOT project and I use a mqtt broker to transport datas from my sensors to a backend and if in the future, I want to use the bus package I can simply dedicate some topics of my broker for that instead of deploying a redis instance. |
Thanks a lot for the PR! |
Hello 👋🏻 ,
I have a few questions concerning my PR:
Currently mqtt tests are run only on the hivemq broker because it's the broker included in the testcontainers package. I'm wondering if it's useful to run tests on other mqtt brokers like EMQX ?
Concerning the
onReconnect
method I can't have the same behavior as Redis, the mqtt client handle automatic reconnexion