chore: more code cleanup surrounding client.end callbacks#1629
chore: more code cleanup surrounding client.end callbacks#1629robertsLando merged 5 commits intomqttjs:mainfrom
Conversation
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Well done with this cleanup! I would like to discuss about the "bugbug"
| // BUGBUG: the client.end callback never gets called here | ||
| // client.end((err) => done(err)) |
There was a problem hiding this comment.
I use mqttjs in lot of my projects and this happend to me too, I always create a MqttClient wrapper library that handles all client stuff, this is how I handle the close usually: https://github.com/zwave-js/zwave-js-ui/blob/master/lib/MqttClient.ts#L154. I think this could happen when for some reason underlig socket is already closed/destroyed or when store cannot be closed for some reason
There was a problem hiding this comment.
I merged #713 into a local branch and it didn't resolve this issue. It is an interesting change though.
One of my goals with these test changes is to get the behavior of client.end well tested before we make any changes to it. Right now, it's just too hard to reason about how client.end might behave and I'm afraid to make any changes to it.
There was a problem hiding this comment.
I agree! Tests are essentials if we want to make improvements to the code
| client.end(() => { | ||
| client.end((err) => { | ||
| assert.strictEqual(pubCallbackCalled && unsubscribeCallbackCalled, true, 'callbacks not invoked') | ||
| server3.close() |
There was a problem hiding this comment.
Does server3.close expects a callback too?
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1629 +/- ##
=======================================
Coverage 86.22% 86.22%
=======================================
Files 13 13
Lines 1321 1321
=======================================
Hits 1139 1139
Misses 182 182 ☔ View full report in Codecov by Sentry. |
2 participants
More test changes surrounding
client.endcallbaacks. One// BUGBUG:comment marks a bug that I found with these changes.