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

chore: update mqtt to latest 5.x and remove unneeded async-mqtt wrapper #975

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

sorccu
Copy link
Collaborator

@sorccu sorccu commented Nov 6, 2024

It seems that the async-mqtt wrapper has not been updated for a few years and our usage does not require it. It's more future proof to just use mqtt directly.

Relevant MqttClient calls have been updated to Async variants.

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Resolves #[issue-number]

New Dependency Submission

While technically mqtt was added to package.json, it was already used indirectly via async-mqtt.

@clample
Copy link
Collaborator

clample commented Nov 6, 2024

We can drop async-mqtt if you think it's the way to go, but I think the subscribe() call needs to be updated:

await socketClient.subscribe(`account/${this.accountId}/ad-hoc-check-results/${checkRunSuiteId}/+/+/+`)

If we switch to the main mqtt client, I think this will no longer return a promise (it will use a callback), so this will not block until the client is subscribed to the topic. We should be careful with this one, since the client needs to subscribe to the topic before the check runs are triggered. Otherwise, there's a bit of room for a race condition - the client triggers checks but misses the results.

Also I think end() will go from returning a promise to using a callback. This one is less important, though.

It seems that the async-mqtt wrapper has not been updated for a few years
and our usage does not require it. It's more future proof to just use mqtt
directly.

Relevant MqttClient calls have been updated to Async variants.
Copy link
Collaborator

@clample clample left a comment

Choose a reason for hiding this comment

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

Looks good!

@sorccu sorccu merged commit 7bab6a2 into main Nov 6, 2024
3 checks passed
@sorccu sorccu deleted the mqtt-5.10.1 branch November 6, 2024 16:37
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.

2 participants