Adding callback to thingShadow.register #83
Conversation
- Update the thing register function with a completion callback - Add new test for the different states (ignoreDelta and persistentSubscribe) - Updated README.md to reflect the new changes
Torsph
force-pushed
the
add_callback_to_thingshadow_register
branch
from
d8b1007
to
caeccf7
Compare
| pending: false | ||
| pending: false, | ||
| pendingDeltaSubscribe: true, | ||
| pendingPersistentSubscribe: true |
There was a problem hiding this comment.
I think it's better not to treat delta ad other shadow topic subscriptions separately. One flag should be enough to tell if the shadow is ready to take actions. If you like, you could modify _handleSubscriptions() so both delta and other topic subscriptions can be handled in one call; the extra benefit of it is one less MQTT message transmission.
In the same vein, callback should also be invoked only once to be deterministic and avoid user confusion.
There was a problem hiding this comment.
Agree. I was thinking about that. I will make the needed changes :)
Torsph
force-pushed
the
add_callback_to_thingshadow_register
branch
from
0d18560
to
8c183c7
Compare
|
Will clean up the branch if it's accepted. How do you like it? rebase into one commit or something else? |
| @@ -526,6 +572,8 @@ function ThingShadowsClient(deviceOptions, thingShadowOptions) { | |||
|
|
|||
| this.unregister = function(thingName) { | |||
| if (thingShadows.hasOwnProperty(thingName)) { | |||
| var topicsObject = []; | |||
There was a problem hiding this comment.
Use topicObjects to be consistent with your prior definition; and perhaps topicSpecs or toipcDefinitions is more readable?
|
Hey, thanks again for working on this. Your second commit looks good overall. If you don't mind, I could pull it in to our internal pipeline and have a reviewed by 2nd pair of eyes after making some minor adjustments? We could then have it released after that. |
|
Do you want me to fix the small issues before you pull it into the internal pipe? |
|
Certainly if you are up for it:) |
|
Sure! Happy to help :) |
|
|
||
| it("should trigger error when a subscription fails", function () { | ||
|
|
||
| var stubTriggerError = sinon.stub(mockMQTTClient, 'triggerError', function(){return true;}); |
There was a problem hiding this comment.
Haven't used sinon that much before, so don't know if I did it the correct way :)
Hope that will do the work. Please just do all the changes needed in your internal review :) |
|
Hi! Do you have any update this PR? :) |
|
Hi Torsph, Thanks for your contributions. These changes are included in our next release cycle and currently going through the internal pipeline. Thanks |
|
Addressed in release v1.0.13. |
Fixes issue #80 if I understood the aws and mqtt implementation correct. The new code waits for the mqtt lib to invoke the callback on ignoreDeltas and persistentSubscribe topics. When the mqtt lib has invoked both callbacks, will the register function invoke the register callback.
Maybe need for some more test cases, but please review and provide with feedback :)