feat: promises support

[robertsLando]

Fixes #1642

[codecov]

Codecov Report

Patch coverage: 77.55% and project coverage change: -0.14% ⚠️

Comparison is base (8521888) 81.23% compared to head (2b45109) 81.09%.

❗ Current head 2b45109 differs from pull request most recent head a881256. Consider uploading reports for the commit a881256 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1644      +/-   ##
==========================================
- Coverage   81.23%   81.09%   -0.14%     
==========================================
  Files          21       21              
  Lines        1327     1365      +38     
  Branches      318      324       +6     
==========================================
+ Hits         1078     1107      +29     
- Misses        175      181       +6     
- Partials       74       77       +3     

Files Changed	Coverage Δ
src/lib/connect/tls.ts	86.95% <ø> (ø)
src/lib/connect/index.ts	81.63% <75.00%> (-1.92%)	⬇️
src/lib/client.ts	85.81% <76.92%> (-0.23%)	⬇️
src/mqtt.ts	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robertsLando]

@vishnureddy17 this is ready for review. I sincerly have no clue why there is a very simple test that fails only on node 20 and I cannot reproduce it on my local instance, it only happens on CI 🤷🏼‍♂️

MQTT.js/test/client.ts

Line 560 in c950c06

it('connect should throw error', async function test() {

UPDATE Seems to work now. Using localhost was not working...

[vishnureddy17]

Looks great, except for my thought on connectAsync. Marking as approved so you can judge whether it's important to change that or not.

[vishnureddy17]

I'm unsure about connectAsync. I feel like all the async functions should just be simple "promisification" of a callback-style function, but connectAsync is doing more than that. Maybe have an equivalent function in callback style?

[vishnureddy17]

Or maybe pick a different name for connectAsync to make it clear that it's doing something different from connect

[robertsLando]

connect doesn't accept a callback so a 'promisified' version of it just makes no sense.

connectAsync does much more because it returns a client just when it actually connects and rejects if there is a connection error, that's how most users expect it to work and what async-mqtt does: https://github.com/mqttjs/async-mqtt/blob/master/index.js#L144

[robertsLando]

Or maybe pick a different name for connectAsync to make it clear that it's doing something different from connect

How would you call it? I tried to keep the compatibility for users using async-mqtt, now they don't need that library anymore

[vishnureddy17]

Hmm I'm actually struggling to think of a name.

I think the problem is that we use connect() to instantiate a client, but I think a better API would have the user call the MqttClient constructor and then call connect() or connectAsync() which would call the callback or resolve with a CONNACK instead of the client.

Since we're trying to keep the API close to the v4 API, I think we should just keep what's here

[robertsLando]

Ok so I'm going to merge this for now and I think I will release a stable v5 after this. What do you think? So we will get more feedbacks