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: Add MQTT transport #445

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

edorgeville
Copy link

The goal of this PR is to implement an MQTT transport.
I based my implementation on the http transport. I got a basic proof-of-concept working, but it looks like I'm forgetting an event listener somewhere, as Jest does not exit, displaying this message:

Jest did not exit one second after the test run has completed.
This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.

I'm pretty new to TypeScript, comments and edits are welcomed and appreciated!

@edorgeville edorgeville changed the title Add MQTT transport feat: Add MQTT transport Oct 1, 2020
@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #445 into master will decrease coverage by 4.71%.
The diff coverage is 72.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
- Coverage   97.02%   92.30%   -4.72%     
==========================================
  Files           7        8       +1     
  Lines         168      208      +40     
  Branches       19       25       +6     
==========================================
+ Hits          163      192      +29     
- Misses          5       16      +11     
Impacted Files Coverage Δ
src/transports/mqtt.ts 72.50% <72.50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfd67fd...3b4d0b1. Read the comment docs.

@BelfordZ
Copy link
Member

@edorgeville looks good! The coverage is good, im not too concern about codecode error above.

However, if you could please rebase, as well as remove the changes from the last commit (changing the published files), then we can get this moving!

@edorgeville
Copy link
Author

@BelfordZ done and done!

@BelfordZ
Copy link
Member

BelfordZ commented Oct 27, 2020

@edorgeville looked over the PR more thoroughly and I have a couple questions.

  1. Shouldnt the transport create and expose a broker? As I understand it, 'broker' is the homologue to server in mqtt land. Is this correct?
  2. in all the other transports, the transport class is establishing this.server. How come the mqtt one establishes this.client?

Perhaps I could entice you to open a PR at https://github.com/open-rpc/client-js/ adding MQTT support for client. At that point, we can use generated clients for MQTT via https://github.com/open-rpc/generator/

Thanks for the PR!

@edorgeville
Copy link
Author

@BelfordZ
MQTT uses a centralized structure. The broker handles all the messages. When someone publishes, the broker distributes the message to every client subscribed to the topic.
This means everybody else is a client, even a server-js instance. The broker should not be bundled with server-js, as server-js joins an existing broker and subscribes to a request/in topic, and publishes back on a response/out topic.
I'll look into making a corresponding PR for client-js soon!

@BelfordZ
Copy link
Member

BelfordZ commented Nov 3, 2020

@edorgeville I have taken liberty to update your branch with mqtt as a broker, as I have a couple people looking to use this asap and want to move it on through.

I tried to make a PR to your fork, but gh kept giving me 404. Somehow im allowed to change your fork.... so I went ahead and did.

Let me know what you think.

@edorgeville
Copy link
Author

I understand in some situation having the broker bundled with server-js makes sense, but it shouldn't be mandatory.

For example, on my current project, we already have a broker which a lot of clients connect to, reporting metrics, mostly.
We want to add with multiple sperate instances of server-js with different roles.

The first one listens to the server1 topic and answers on server1/response.
The second one listens on server2 and answers on server2/response.

Both instances connect to the same broker. Other clients connect to the same broker and communicate with each other as well as with server1 and server2. It would not make sense to have multiple brokers.

@BelfordZ
Copy link
Member

BelfordZ commented Nov 4, 2020

@edorgeville thanks for the review. While I understand that this doesn't fit the bill for your particular use case, I think that having server-js provide the ability to create a broker is highly useful.

For your particular usecase, I think the best approach would be to have https://github.com/open-rpc/client-js support MQTT transport over ws/tcp/tls. This way, we could then use https://github.com/open-rpc/generator to produce a typed client for your API. At this point, setting up a server that runs a node program to use said generated client, and provide your above usecase, becomes very clean.

As for server-js implementing the broker side, do you see any issues with the impl here?

@edorgeville
Copy link
Author

I will explain in as much detail why bundling the broker with server-js is not always a good idea.

Let's take for example an IoT project where you have a fleet of electric scooters that users can ride.
Each scooter is equipped with a single-board computer (think Raspberry Pi). Those computers connect to a single MQTT broker (for example test.mosquitto.org). Each runs a server-js instance with the same methods. Each subscribe to their own topic: scooters/{uuid}.

Now, when a user wants to unlock scooter 12345 through the app, the app connects to the broker and sends the following message on topic scooters/12345:

{
    "jsonrpc": "2.0",
    "method": "unlock",
    "params": [],
    "id": 0
}

The scooter answers back on topic scooters/12345/response:

{
    "jsonrpc": "2.0",
    "result": {
        "message": "Scooter unlocked successfully."
    },
    "id": 0
}

Now. This only works because there is one broker which every scooter and users connects to. With your proposition, you would have to keep track of every single scooters IP address, which the application would need to connect to in order to send their request.

@edorgeville edorgeville marked this pull request as ready for review November 5, 2020 18:45
@edorgeville
Copy link
Author

Hey @BelfordZ, would you like me to make a proposal for implementing both solutions at the same time?
Maybe an option broker: internal/external or similar?

@hholst80
Copy link

I understand in some situation having the broker bundled with server-js makes sense, but it shouldn't be mandatory.

I work professionally with MQ brokers and for the record, bundling a MQTT broker with server-js does not make any sense to me.

As far as a messaging middleware is concern they are all clients: producers, consumers, or both.

Compare with loopbacks view on this.

https://strongloop.com/strongblog/introducing-strongloops-unopinionated-pubsub/

@edorgeville
Copy link
Author

Hi @BelfordZ, would love to get this conversation back on the rails.

@BelfordZ
Copy link
Member

BelfordZ commented Apr 16, 2021

Hey! Thanks @edorgeville for the detailed explanation. That makes a lot of sense. I was thinking that the intent was to use server-js as the https://test.mosquitto.org/ api in your example.

That said - I get it now. Here, we are trying to use server-js & client-js on a single device, giving bi-directional async comms. The goal for server-js & client-js has been to evenutally abstract away the whole 'server on the client' stuff. Since the JSON-RPC spec makes it clear that there is ultimately no distinction between what is a client and what is a server outside inspecting a single request. When you consider this + the precense of notifications (requests which expect no response), JSON-RPC well accomodates bi-directional comms. OpenRPC, however, currently does not have a good way to model this. The only way to do this would be to have 2 seperate documents (or the same document if they happen to both have the same methods), and run both client-js & server-js, as you are trying to do.

One of proposals on the table right now is to add the ability to describe a method as being client -> server or client <- server. If this functionality was available, you would be able to describe your scooter API in a single document, and the scooters would be able to know what messages to listen for, and vice versa.

Since none of that is in/decided on yet, I think your idea here is very reasonable. If you want to wait until the dust settles on the above problem space, and then get a PR in for mqtt server / client, or if you want to just get this together and merged, both are fine by me. If you do want to get this PR in - I defer to your expertise. I just ask that you help to keep it maintained, and that we can agree to revisit this if/when we deliver on the notifications/bidirectional upgrades.

Thanks for your patience on this one. I'm still very excited about using MQTT & JSON-RPC with OpenRPC :)

@edorgeville
Copy link
Author

Glad to see you back! If you are confortable with merging this PR in it's previous state (reverting your last two commits), I'd be happy to help maintain it. No issue either with revisiting when bidirectional lands.

@natcl
Copy link

natcl commented May 9, 2022

Curious to see if there were any updates on this ?

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.

4 participants