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

Add onClosed event handler. #16

Merged
merged 1 commit into from
Jan 25, 2017
Merged

Add onClosed event handler. #16

merged 1 commit into from
Jan 25, 2017

Conversation

ShaneMcC
Copy link
Contributor

Closes #12

Happy to re-work this one if needed.

Opted to go for onClosed rather than onReadyStateChange as that was more useful.

I wasn't quite sure where to put the onClosed call so put it in close() and in connect(), in both cases it only fires if the previous state was OPEN so there shouldn't be any cases of it double-firing.

This means that the event will fire if the user calls close(), or if the socket ends up getting closed by other means.

Only putting it in connect() seemed mean that a user calling close() didn't get the onClosed event fired quickly as it waited until there was some data on the socket to read before it ultimately got round to calling the finally block.

@jkodumal
Copy link
Contributor

This looks good to me. It's an API change on the EventHandler interface so we'll have to version this as a major change when we release.

@jkodumal jkodumal merged commit 1f09ffc into launchdarkly:master Jan 25, 2017
arun251 pushed a commit that referenced this pull request Apr 4, 2018
Add support for custom max backoff time
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.

Add onClose or onReadyStateChange event
2 participants