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 a 'closed' event #73

Merged
merged 1 commit into from
Mar 29, 2016
Merged

add a 'closed' event #73

merged 1 commit into from
Mar 29, 2016

Conversation

rkadyb
Copy link
Contributor

@rkadyb rkadyb commented Mar 14, 2016

I ended up needing an 'after-close' event for a project I'm working on, figured I'd share this in case it's worthwhile for anyone else.

Reasoning:

The existing :closeevent is fired BEFORE the websocket connection is actually closed, which makes sense if you want to send a message / use the API immediately before disconnect. This isn't as helpful if you want to do something not related to the API AFTER the disconnect.

@dblock
Copy link
Collaborator

dblock commented Mar 14, 2016

Looks good. Needs CHANGELOG, tests and a README update please. Maybe call it :closed instead?

@dblock
Copy link
Collaborator

dblock commented Mar 25, 2016

Bump, maybe you want to finish this one @rkadyb ?

@rkadyb
Copy link
Contributor Author

rkadyb commented Mar 26, 2016

Sorry for the delay, will finish this one up shortly!

@rkadyb rkadyb changed the title add an 'after-close' event add a 'closed' event Mar 29, 2016
@rkadyb
Copy link
Contributor Author

rkadyb commented Mar 29, 2016

I cleaned this PR up a bit, please let me know what you think.

@dblock
Copy link
Collaborator

dblock commented Mar 29, 2016

I like it! Squash it and I'll merge?

changelog entry

updated readme, real_time example

add integration test
@rkadyb
Copy link
Contributor Author

rkadyb commented Mar 29, 2016

Squashed, thanks!

@dblock dblock merged commit 9a1245a into slack-ruby:master Mar 29, 2016
@dblock
Copy link
Collaborator

dblock commented Mar 29, 2016

I merged. Before the next version ships, maybe we want a proper callback mechanism added here where you can do things before, after and around something, ActiveSupport style. Maybe you might want to take a look @rkadyb.

@dblock
Copy link
Collaborator

dblock commented Apr 1, 2016

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