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

Ensure we reconnect with the same options passed to start #347

Merged
merged 4 commits into from
May 17, 2017

Conversation

CharlieHess
Copy link
Contributor

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've written tests to cover the new code and functionality included in this PR.
  • I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

This PR handles an issue with the RTM client wherein reconnects do not use the same arguments originally given to rtm.start. So, if you specify something like simple_latest, it'll work the first time, but reset to default once you reconnect.

Related Issues

There was no issue filed for this bug. This PR fixes an unrelated issue with the README, see #300.

Test strategy

I added a test that reproduces this case.

@codecov
Copy link

codecov bot commented May 17, 2017

Codecov Report

Merging #347 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage    85.8%   85.99%   +0.18%     
==========================================
  Files          44       44              
  Lines        1205     1207       +2     
  Branches      178      178              
==========================================
+ Hits         1034     1038       +4     
+ Misses        171      169       -2
Impacted Files Coverage Δ
lib/clients/rtm/client.js 72.79% <100%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f46169...3968f9c. Read the comment docs.

@@ -175,7 +181,7 @@ RTMClient.prototype._lastPong = 0;


/**
*
* A running count of socket connection attempts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are a saint for fixing things like this 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i gotchu dawg

Copy link
Contributor

@aoberoi aoberoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome!

@aoberoi aoberoi merged commit 2b8ee33 into slackapi:master May 17, 2017
@CharlieHess CharlieHess deleted the reconnect_fixes branch May 17, 2017 01:31
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