Js Mqtt - PR #7 Client implementation#703
Conversation
| * Emitted when the client successfully establishes an MQTT connection to the remote endpoint | ||
| */ | ||
| export interface ConnectionSuccessEvent { | ||
| connack: mqtt5_packet.ConnackPacket, |
There was a problem hiding this comment.
Not needed technically. Even if we did include it, I'd make it a clone of the locally-computed copy so that protocol state wasn't externally modifiable. Nothing stops us from adding it later if a reason comes up.
sbSteveK
left a comment
sbSteveK
left a comment
There was a problem hiding this comment.
Some minor comments on the resubscribe stuff. Will move onto bulk of client next.
| this.subscriptions.clear(); | ||
| } | ||
|
|
||
| onSubscribeRequest(topicFilter: string, qos: mqtt5_packet.QoS) { |
There was a problem hiding this comment.
Edge case:
User is offline with OfflineQueuePolicy set to never. Subscribe stores this before checking the policy. It's discarded. Once connection is established we send out a subscribe based on buildResubscribePacketList that shouldn't be sent due to OfflineQueuePolicy.
We submit a subscription but it's rejected (maybe permissions) but the sub is set before we process the suback. On a reconnect we attempt to resub despite it having failed the first time so we aren't really "resubscribing". This initial failure that is followed later on in a discon/recon may be a success due to some kind of permission or broker change. This one is less an issue but still probably possible.
There was a problem hiding this comment.
Disagree that this is an issue. I think of this as capturing "intent". If the user wants to subscribe to X we will try to subscribe to X. If it fails, it fails, but by triggering off of subscribe/unsubscribe we stay true to original intent without having to track acks or additional bookkeeping that I don't think buys us anything useful.
| this.subscriptions.set(topicFilter, qos); | ||
| } | ||
|
|
||
| onUnsubscribeRequest(topicFilter: string) { |
There was a problem hiding this comment.
We could probably use this to remove a subscription that failed if we get a negative suback for the second issue listed above.
| */ | ||
| EnabledOnSessionResumptionFail, | ||
|
|
||
| Default = 0, |
There was a problem hiding this comment.
The default mode seems not be used at all.
There was a problem hiding this comment.
While it's internal, the basic intention of the client interface is to be as if it were public.
3 participants
Client implementation
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.