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

Input requested: Future of MQTT.js #1541

Closed
BertKleewein opened this issue Sep 30, 2022 · 26 comments
Closed

Input requested: Future of MQTT.js #1541

BertKleewein opened this issue Sep 30, 2022 · 26 comments

Comments

@BertKleewein
Copy link
Contributor

I'm seeking input on an idea for moving this repo forward.

My team at Microsoft uses MQTT to communicate with the Azure IoTHub service. __MQTT.js does an excellent job here, but, frankly, it's starting to show its age. It was written when the node.js ecosystem was less mature, and it has evolved as the ecosystem changed. Many (but not all) of these "evolutionary steps" were bolted-on pieces of technology that "added a little more to MQTT.js". The result is code that does what it needs to do, but it’s hard to understand and even harder to change.

@YoDaMa and @vishnureddy17 looked at this problem and started to build an updated version of this library from the ground up. This is a noble pursuit. It’s also extremely easy to underestimate how difficult it is. While MQTT, the transport, is relatively simple, libraries with thousands of dependents, like MQTT.js, are not so simple. The thousands of dependents, or customers, or users, or whatever you want to call yourselves are what make MQTT.js powerful. "Rebuilding from the ground up" means "moving forward by moving away from these customers." I personally do not like this idea.

Instead, we should take the time and take a large evolutionary step to bring the existing MQTT.js code "into the modern age." We should call this MQTT.js version 5.0.0, with the major version change from 4 to 5 showing that this is a breaking change. But we should only break things when there is a clear advantage. Changes in API or behavior from version 4.7.3 to version 5 should be justified and documented. Customers who want to "play it safe" can use semantic versioning rules in package.json to make sure they do not get version 5. Other customers who want to "follow along" shouldn’t need to make major changes, but we cannot guarantee this.

We should convert this code to typescript. I have done this as a POC (proof-of-concept). The extra type information has shown me so many places where the code is sloppy and could be tightened up, especially around options and client properties and callback signatures. You can see my POC here

We should also start using prettier and eslint on the code. I hate arguing about "what format is correct" for pretty-printers. I love being able to write code without worrying about formatting because I know it will get automatically formatted. Unfortunately, this means that every line of code gets changed, and merges from "pre-prettier" code into "post-prettier" code gets ugly. I wish I knew a way to avoid this.

We should start publishing commonjs and ECMAScript side-by-side if we can. It's too bad that there is still a distinction here, but I think we can make both sides happy without picking a side.

We need to make sure the browserified packages still work, but I assume they will change. I'm less clear on details here, but I assume that webpack and ECMAScript is the way that people want to go. I could be wrong, and we have options here.

If we can do these things, then the code will be ready for some interesting changes. I have a ton of ideas and opinions here, but I do not want to cloud the discussion just yet. :)

I'm curious to hear what people think. Some of the things I have said here are strong beliefs. Others just seem like the right path.

Tagging an arbitrary list of active/recent contributors to get your attention. Apologies for the spam.
@mcollina, @vishnureddy17, @YoDaMa, @redboltz, @scarry1992, @radekg, @ysfscream, @sublimator, @RangerMauve, @4rzael, @anhldbk, @bkp7, @nosovk, @robertsLando, @ogis-yamazaki, @ryanwinter, @anthonyvercolano

@robertsLando
Copy link
Member

Agree that this repo needs a fresh new look, happy to help where I can!

@nosovk
Copy link
Contributor

nosovk commented Oct 1, 2022

We moved to https://github.com/shanewholloway/js-u8-mqtt for frontend apps. It works in Vite/Svelte without problems. Also it's tiny. It lacks of some features, like ACK control, but it's very specific usecase.

@radekg
Copy link
Contributor

radekg commented Oct 1, 2022

@BertKleewein
Copy link
Contributor Author

@BertKleewein have you seen this? https://github.com/mqttjs/MQTT.js#next-major-version-of-mqttjs

@radekg, Yes, thank you. I actually work directly with the authors of that repo. This is a different path, but the goal is the same. We're putting that project on pause while we see if we can accomplish our goals in a less ambitious way.

@radekg
Copy link
Contributor

radekg commented Oct 1, 2022

@BertKleewein I’d say go for it.

@BertKleewein
Copy link
Contributor Author

We moved to https://github.com/shanewholloway/js-u8-mqtt for frontend apps.
@nosovk - thanks for the link. That looks interesting.

@nosovk
Copy link
Contributor

nosovk commented Oct 2, 2022

We moved to https://github.com/shanewholloway/js-u8-mqtt for frontend apps.
@nosovk - thanks for the link. That looks interesting.

Actually I should tag @shanewholloway, probably he can suggest how to migrate to modern approach (or better rewrite all from scratch).

@mcollina
Copy link
Member

mcollina commented Oct 2, 2022

Converting this library to TypeScript, publishig it as ESM, etc does not solve any maintenance issues this module has (which has led to me stopping maintaining it). Fundamentally, MQTT.js has to be rewritten to fix all the problems this library has.

Anyway, I stopped maintaining this library long ago, so I'd leave any decisions to @YoDaMa.
This makes an issue completely within Microsoft and Azure, I guess there is budget to maintain this library... which makes things significantly easier.

@robertsLando
Copy link
Member

robertsLando commented Oct 3, 2022

Converting this library to TypeScript, publishig it as ESM, etc does not solve any maintenance issues this module has

@mcollina Could you be more specific about this? Would be interesting to know which, in your opinion, are the main issues with this library

@mcollina
Copy link
Member

mcollina commented Oct 3, 2022

It's a bit hard to list here, I fully briefed @YoDaMa at the time. There are 300+ open issues, too :/.

@shanewholloway
Copy link

We moved to https://github.com/shanewholloway/js-u8-mqtt for frontend apps.
@nosovk - thanks for the link. That looks interesting.

Actually I should tag @shanewholloway, probably he can suggest how to migrate to modern approach (or better rewrite all from scratch).

Thanks for the compliment @nosovk -- glad to hear you're using it. Feature/pull requests requests are welcome regarding the "ACK control feature" mentioned above.

I created u8-mqtt-packet to be a ESM-based browser-first library that only parsed MQTT packets. The idea being that one could make one-off single purpose MQTT clients simply; e.g. a web-hook to MQTT update relay, or a service-worker MQTT intermediary. Developed directly from the MQTT 3.1 and MQTT 5 specs, I developed and ran my test suites against the Mosquito and eJabberD MQTT servers in both 3.1 and 5 protocol modes. The packet library was designed to be lightweight for the browser, as well as work in Deno and NodeJS ecosystems. My u8-mqtt client library puts the necessary pieces together for the use cases I needed -- and to get that iterative development outside of the more stable packet library.

All that to say, I'd be happy if an MQTT.js client compatible fork chose to leverage these ESM packet parsing libraries. I'm not a TypeScript fan, but I'd be happy to publish static types alongside the package if someone creates a pull request for it.

@BertKleewein
Copy link
Contributor Author

@mcollina, @robertsLando

Converting this library to TypeScript, publishig it as ESM, etc does not solve any maintenance issues this module has

Agree that Typescript and ESM by themselves don’t solve any problems. Typescript helps to highlight sloppy programming and helps to enforce less sloppy programming in the future. ESM is a byproduct that makes the library more attractive to some customers.

I’ve started to make a list of the things that are “wrong” with this code. In short, this code is a mess. It’s hard to work on. There’s no way to know that you didn’t break something. It’s easy to break things.

In the end, I think (or hope) that we’ll end up with a piece of code that looks like a total rewrite. I’m viewing this like a “tear it down to the studs” renovation.

Here’s a few notes on things I’ve noticed. This is not complete and some of them probably don’t make sense to anyone but me.

(I mean no disrespect to anyone who has worked on this code. I try very hard to practice egoless coding -- "you are not your code")

testability

  1. The tests we have rely on a fake MQTT server. Understanding existing tests means keeping both the client and server in your head and being able to debug both at the same time.
  2. There is no way to test against live servers (aedis, mosquito, etc).
  3. In-browser testing is extremely limited and clearly hasn’t been run recently.
  4. Some code is just not tested at all.

client.js

  1. Client class is just too big and poorly organized. It’s a big mass of spaghetti that needs to be detangled.
  2. Some code clearly tacked on, like topic alias support which was added as static helper functions outside the client class.
  3. Options are not well defined. Some are stored in options struct, others in client, with no definition or consistency.
  4. Many individual state variables (such as MqttClient.disconnecting and MqttClient._pingResp) not documented and not consistently implemented.
  5. MqttClient.end() is a mess.
  6. EventEmitter handlers used for internal flow, such as this.on(‘connect’ and this.on(‘close’, make code flow extremely difficult to reason about. This is part of why MqttClient.end is so messy.
  7. Outgoing queue and store queue, which are two different things, have poorly defined structure. It’s hard to know what is stored in each queue outside of a debugger.
  8. In short, you cannot read client.js and have any idea or how it will behave at runtime. The only way to know how it’s going to work is to run it. There is no way to confidently make changes to the code.

@mcollina
Copy link
Member

mcollina commented Oct 3, 2022

I agree on all accounts.

@robertsLando
Copy link
Member

@BertKleewein What is the implementation status of https://github.com/mqttjs/mqttjs-v5?

We're putting that project on pause while we see if we can accomplish our goals in a less ambitious way.

What you mean with less ambitious way?

@BertKleewein
Copy link
Contributor Author

@robertsLando -

The mqttjs-v5 repo is on hold for the moment.

What you mean with less ambitious way?

The previous (more ambitious) goal was to write mqttjs-v5 from the ground up, as a fully modernized promise-first library based on the MQTT spec. Features, interfaces, behaviors, and tests would all be written from scratch.

The current goal is to modernize the current implementation, but keep as many of the current features, interfaces, behaviors, and tests as possible.

@vishnureddy17
Copy link
Member

As @BertKleewein mentioned, development of https://github.com/mqttjs/mqttjs-v5 is on hold. I've updated the README and made a small writeup in an issue over there to make this clear.

@robertsLando
Copy link
Member

@BertKleewein What about starting with converting it to TS by keeping all stuff as it is (but typed) and then simply start making code improvements starting from that state?

I have converted to TS lot of my old projects and this helped me a lot as once things are typed TS helps you to keep changes consistent

@woifes
Copy link

woifes commented Apr 13, 2023

Dont mind me I am just an ordinary guy who uses this library in most of his fun projects. Therefore I am not checking in regularly but when I do this happens... It makes me sad, worried and a little bit angry.
It is very surprising to me that a library which has about 500.000 weekly downloads and 2344 dependents is left alone for so long. Especially because the platform Nodejs and the Web is extremely widely used and MQTT is one of the most important protocols of today in IoT. But I want to explicitly not blame the contributors of the library for this. You did a good job so far, well done!

What makes me angry is, that in this 500.000 weekly downloads I assume there is a good amount of people and for sure also companies who have enough resources to help out here. I dont want to exclude myself of this I think we can do better.

@shanewholloway Your library seems very promising and props to you for the effort so far. On the other hand I hope you change your mind about typescript. Shipping .d.ts files besides a javascript code base seems very old fashioned to me.

Ok thats was the non productive part. On the current question I think the building from ground up approach is the better way.

@robertsLando
Copy link
Member

As I said I'm in helping with this but I still don't understand what's the plan as I see nothing is moving forward in last months

@mcollina
Copy link
Member

What makes me angry is, that in this 500.000 weekly downloads I assume there is a good amount of people and for sure also companies who have enough resources to help out here.

Sadly, this is not true. Very few people wants to do the work to maintain this library, and even less companies (including AWS and to some extent Microsoft) wants to sponsor this work.

I got burned out by maintaining this library a long time ago.

@woifes
Copy link

woifes commented Apr 16, 2023

Sadly, this is not true. Very few people wants to do the work to maintain this library, and even less companies (including AWS and to some extent Microsoft) wants to sponsor this work.

I got burned out by maintaining this library a long time ago.

What a shame :(

As I said I'm in helping with this but I still don't understand what's the plan as I see nothing is moving forward in last months

I am not so used to project management with github and higher github features in particular. But I suppose we "unhold" the mqttjs-v5 and setup this project feature which seems to be like kanban. There we can define some milestones, setup ADRs, unit tests and integration tests etc. Maybe @vishnureddy17 can give us insight what was the last thing they wanted to tackle so one can start of this point if they want to.
Basically I think we should move this discussion to the other repo.
About the current repo I know how hard this sounds but I think we should leave it. It seems to work for about 500.000 weekly downloaders in a way that they dont seem to have the need tp check in here. So it can stay for now. The open issues are invited to help out in the new repo or open their issue there again.

@pglombardo
Copy link

Sadly, this is not true. Very few people wants to do the work to maintain this library, and even less companies (including AWS and to some extent Microsoft) wants to sponsor this work.

@mcollina I don't see any "How to Sponsor" information in the repository. Every stakeholder that would likely sponsor is in this issue and repository... Just a thought.

@mcollina
Copy link
Member

I would be happy to coordinate things if there was a commitment of at least ~100k €. This would enable somebody to work on this mostly full time for a while and fund any extra work that would be needed. (not me, I have my own startup to work on).
I can help setting up an opencollective to manage the funds.

I'm also ok in helping getting the rewrite shipped if a company is willing to dedicate 1-2 engineers full time for it for 3-6 months, and take the burden of bugfixing afterwards.

@ryanwinter
Copy link
Contributor

ryanwinter commented May 1, 2023

Sorry for the delayed response to this topic, we wanted to make sure we were aligned internally.

Our team at Microsoft has contributed to v4 releases over the past few years. We have recently undergone some major restructuring, and as part of this, team members have been moved to different projects leaving us with no dedicated funding for the MQTT.js project.

At this time, we are planning the following:

  1. MQTT.js is still a key component of the Azure IoT Node SDK, and we will continue to contribute fixes as required to support our product.

  2. MQTT is a key part of the new generation of Azure services that the company is building. As these services mature, we will revisit existing language client support, including MQTT.js, to understand how they fit into the product strategy. Unfortunately, we don't have a timeframe for this yet, but my best estimate would be 6 - 12 months from now.

Our team is disappointed that this is where we ended up, and we understand that other users of the project may need to keep moving the project forward without our support. We are hopeful that we can rejoin the community again in the future.

@vishnureddy17 vishnureddy17 pinned this issue May 1, 2023
@vishnureddy17 vishnureddy17 changed the title Input requested: updating the code and porting to TypeScript Input requested: Future of MQTT.js May 1, 2023
@robertsLando
Copy link
Member

robertsLando commented Jun 27, 2023

Hi everyone. I recently started helping maintaining this library.

I just released a v5.0.0 beta v5.0.0-beta.0 (release)

Also available on NPM (npm i mqtt@5.0.0-beta.0): npmjs.com/package/mqtt/v/5.0.0-beta.0

Any feedback would be more then welcome! Also if you think there could be other breaking changes to add this would be the right moment to do them. For now I just dropped support for EOL nodejs versions.

Thanks

@robertsLando
Copy link
Member

Just a quick follow up here:

  • Dropped support for EOL NodeJS version
  • Tests are no longer flaky
  • Browser tests are now working (don't think they ever worked) and have been added to CI
  • All deps have been bumped to latest fixing many security issues (and minor bugs)
  • StandardJS has been replaced with eslint+prettier
  • Project has been completely rewritten in Typescript. This improved types a lot, I think I added at least 10/15 props to ClientOptions that were not listed, also many code refactor has been done to make everything more readable
  • Promises support has been added for all public methods (that have a callback).
  • Many other bug fixes

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

No branches or pull requests

10 participants