Skip to content
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

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

MaximeMRF
Copy link
Contributor

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

const data = { test: 'test' }

await transport.subscribe('testing-channel', (payload) => {
assert.deepEqual(payload, data)
Copy link
Collaborator

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

https://japa.dev/docs/plugins/assert#plan

Copy link
Contributor Author

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.

Copy link
Collaborator

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

@Julien-R44
Copy link
Collaborator

Julien-R44 commented May 25, 2024

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 ?

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

@MaximeMRF
Copy link
Contributor Author

Hello @Julien-R44,
sorry i've not work on this pr since a long time but now i'm back
i will update the readme once you will say me that's the code is legit to a merge

@RomainLanz
Copy link
Member

RomainLanz commented Sep 2, 2024

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?

@MaximeMRF
Copy link
Contributor Author

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 MqttTransportConfig to support the acknowledgments. So all messages sended by the producer will have the same quality of service.

@RomainLanz
Copy link
Member

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?

@MaximeMRF
Copy link
Contributor Author

Ah ok my bad.
Yes it seems to, but my pr is here to support the mqtt protocol as well as mqtt broker but not amqp and broker like RabbitMQ and Kafka.
And by default mqtt has no acknowledgments so it fit your requirements

@Julien-R44 Julien-R44 marked this pull request as ready for review October 1, 2024 21:57
@Julien-R44
Copy link
Collaborator

Is this PR ready @MaximeMRF? Looks all good to me 👍

Any additional comments @RomainLanz, or can we merge this?

@MaximeMRF
Copy link
Contributor Author

@Julien-R44 hi before merge i have to provide the docs

@RomainLanz
Copy link
Member

All questions are answered on my side!

Do you plan to use it in your project @MaximeMRF?

@MaximeMRF
Copy link
Contributor Author

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.

@Julien-R44
Copy link
Collaborator

Thanks a lot for the PR!

@Julien-R44 Julien-R44 merged commit cdea48c into boringnode:main Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants