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

refactor: callbacks on end() #1080

Merged
merged 1 commit into from
Apr 25, 2020
Merged

refactor: callbacks on end() #1080

merged 1 commit into from
Apr 25, 2020

Conversation

YoDaMa
Copy link
Contributor

@YoDaMa YoDaMa commented Apr 25, 2020

There are a few historical PRs that I looked through related to this PR: https://github.com/mqttjs/MQTT.js/pull/771/files
https://github.com/mqttjs/MQTT.js/pull/827/files

This PR addresses some of the flakiness in tests seen in the build system, both by adding a minor adjustment to a test but more importantly by adjusting the way end() calls it's callbacks.

If the end() method is disconnecting, it will simply return and never invoke the callback passed to it... This seems like flawed behavior since the callback passed is unique to that call to end(). This PR changes how end() works, so that the callback is invoked regardless if the client is disconnecting or not. Additionally, it removes the use of arguments that was introduced in the above two PRs, as I could find no logical justification for the use of arguments over actual defined parameters? Finally, I could find no logical justification of just returning the arguments object as a parameter of the callback, so that is changed where the callback is simply invoked.

I thought about adding information into the callback, like if the client is disconnecting, connected, or disconnected, but because the end event is emitted once the stores are closed we can use that in tandem with the callback to determine if the client is actually disconnected. If the callback is invoked on a call to end but the end event has not yet been emitted then the user can know that the client is in the process of disconnecting but has not fully disconnected yet.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@YoDaMa YoDaMa merged commit 231682d into mqttjs:master Apr 25, 2020
PremiumBurger pushed a commit to quickstar/MQTT.js that referenced this pull request Nov 27, 2020
@YoDaMa YoDaMa deleted the mqtt5_updates branch February 10, 2022 15:21
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.

None yet

2 participants