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

onConnect/onDisconnect should be triggered only on the connection state transition #30

Open
ppetrosh opened this issue Feb 2, 2019 · 5 comments

Comments

@ppetrosh
Copy link

ppetrosh commented Feb 2, 2019

  • onConnect is now called either when connect is called or when the state transitions (from offline to online)
  • this behavior contradicts the documentation https://github.com/electricimp/ConnectionManager#parameters-2 Called when ConnectionManager’s connection state changes from offline to online. The callback function has no parameters. (However, in the rest of the places it's properly described).

I believe onConnect should be called only when the transition happens or there should be a way to distinguish connection from just the connect call (when already connected).

There are multiple options for how to address this:

  • add a new onStateChange (name TBD) callback - backward compatible
  • add a flag to the existing onConnect callback - breaking change
@blindman2k
Copy link
Contributor

It's was quite intentional that the onConnect handler fires when there is an existing connection. If you are going to change the behaviour then maybe make the new behaviour optional. cm.connect(false) or alike.

@betzrhodes
Copy link

I like the idea of adding a feature that allows the user to check if the connection state has changed. The 2 proposed options don't look like they change the current behavior.

Adding a callback -
Pros: Backwards compatible.
Cons: Could be confusing - what callbacks are triggered and in what order (onStateChange, onConnect, onNextConnect).

Adding flag -
Pros: Usage seems straight forward.
Cons: Breaking change.

Of these 2 options I like the addition of the flag more.

@ppetrosh
Copy link
Author

ppetrosh commented Feb 4, 2019

@blindman2k would you mind us adding a "state changed" flag to the onConnect callback? It's going to break the code but would leave the behavior the same.

@blindman2k
Copy link
Contributor

My philosophy is that you should only break existing code if everyone is going to want it. I don't think many people will want this change so I don't support it. Having said that it doesn't offend me :)

@ppetrosh
Copy link
Author

ppetrosh commented Feb 5, 2019

I would agree with you... Let me think.

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

No branches or pull requests

3 participants