-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: promises support #1644
Merged
Merged
feat: promises support #1644
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
247d614
feat: promises support
robertsLando 0cb9b77
feat: add async methods
robertsLando 6e150db
fix: client async tests
robertsLando 7edbe7e
fix: safer publish
robertsLando 36d3f15
Merge branch 'main' into feat-promises
robertsLando 1e9dea0
fix: broken tests
robertsLando a39c3c8
fix: publish check
robertsLando 4efa94c
fix: add subscription check
robertsLando 24a34cd
fix: move async tests
robertsLando 1544f40
feat: connect/publish errors tests
robertsLando 2f3b844
docs: add docs
robertsLando fe93fdf
docs: add missing link
robertsLando de44647
fix: weird test result with node 20
robertsLando c950c06
fix: failing test
robertsLando a881256
Merge branch 'main' into feat-promises
robertsLando File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about
connectAsync
. I feel like all the async functions should just be simple "promisification" of a callback-style function, butconnectAsync
is doing more than that. Maybe have an equivalent function in callback style?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe pick a different name for
connectAsync
to make it clear that it's doing something different fromconnect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whatasync-mqtt
does: https://github.com/mqttjs/async-mqtt/blob/master/index.js#L144There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you call it? I tried to keep the compatibility for users using
async-mqtt
, now they don't need that library anymoreThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theMqttClient
constructor and then callconnect()
orconnectAsync()
which would call the callback or resolve with aCONNACK
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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