fix: publish/subscribe/unsubscribe types and missing types exports

[dgg]

Fixes some of the issues mentioned in #1686.

In particular:

1. exporting missing types from callbacks
2. missing callback argument

I tried to figure out how to fix 2) and 3) (inconsistent error argument declaration in callbacks and runtime/typing mismatch) but it goes deeper into the library (writing to streams) than I am comfortable with.
Maybe it is easier than I think, but I believe it needs some more overview of the project than I have.

[robertsLando]

It's not very clear to me where in the code we return the packet in publish method...

Publish calls _sendPacket that then calls _writePacket that simply calls cb on end without any arg

[dgg]

It's not very clear to me where in the code we return the packet in publish method...

Publish calls _sendPacket that then calls _writePacket that simply calls cb on end without any arg

Yes, I agree with you it is not very obvious when reading the code, but if you look at the tests publishing > should fire callback ... in test/abstract_client.ts it is undeniable that the callback is called with a non-falsy packet argument: either a publish cmd for QoS 1 or a pubrel for QoS 2:

      ✔ should fire a callback (qos 0)
QOS 1 {
  cmd: 'publish',
  topic: 'a',
  payload: 'b',
  qos: 1,
  retain: false,
  messageId: 30300,
  dup: false
}
      ✔ should fire a callback (qos 1)
QOS 1 ERR {
  cmd: 'publish',
  topic: 'a',
  payload: 'b',
  qos: 1,
  retain: false,
  messageId: 27647,
  dup: false
}
      ✔ should fire a callback (qos 1) on error
QOS 2 { cmd: 'pubrel', qos: 2, messageId: 15779 }
      ✔ should fire a callback (qos 2)
QOS 2 ERR { cmd: 'pubrel', qos: 2, messageId: 12513 }

(I console-logged the packet in the callback invoked in the tests).

Besides, that runtime behavior of calling the callback with the packet was the behavior in v4 of the library.

[robertsLando]

@dgg Wondering if it just would make more sense to don't return the packet when doing a publish at all. What is the possible use case behind this considering it's not always defined?

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (316d9a2) 81.09% compared to head (29ab2fb) 81.09%.

❗ Current head 29ab2fb differs from pull request most recent head 0e776c9. Consider uploading reports for the commit 0e776c9 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1688   +/-   ##
=======================================
  Coverage   81.09%   81.09%           
=======================================
  Files          21       21           
  Lines        1365     1365           
  Branches      322      322           
=======================================
  Hits         1107     1107           
  Misses        181      181           
  Partials       77       77           

Files Changed	Coverage Δ
src/lib/client.ts	85.83% <100.00%> (ø)

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

[dgg]

@dgg Wondering if it just would make more sense to don't return the packet when doing a publish at all. What is the possible use case behind this considering it's not always

I do not know about the use case (maybe the one that created the function in the first place or someone that actually uses it can) but in our case we would have to change quite a few places in which we were declaring the argument.

In reality it is a matter of backwards compatibility. There was a function with a callback signature that was not marked as deprecated or anything and it goes away in the next major release (fair, but better warn beforehand) but in reality, the runtime behavior is maintained, only the declaration of the types changed.

[dgg]

Btw, I can see that the x20 build is broken. I was going to ask how to fix the issues, but I figured out already.

[robertsLando]

5.0.5 is coming