-
Notifications
You must be signed in to change notification settings - Fork 182
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
Adapt to Slack API changes #235
base: master
Are you sure you want to change the base?
Conversation
@@ -42,6 +42,6 @@ defmodule Slack.Rtm do | |||
|
|||
defp slack_url(token) do | |||
Application.get_env(:slack, :url, "https://slack.com") <> | |||
"/api/rtm.start?token=#{token}&batch_presence_aware=true&presence_sub=true" | |||
"/api/rtm.connect?token=#{token}&batch_presence_aware=true&presence_sub=true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change start
to connect
here, could that cause issues for people who are still relying on the old start
behavior?
If that's the case, I wonder if it makes sense to make a new connect
method that uses rtm.connect
while keeping the start
method that keeps using rtm.start
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What led to this is that I noticed that my bot was fully broken, and rtm.start
always returned a 429 rate limit error, even with a single request. I interpreted that as Slack turning off the API.
Perhaps that behavior differs for older Bots though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to address @BlakeWilliams 's concerns because we saw this before with a previous iteration I did when trying to switch over to rtm.connect
Ok, so maybe make this configurable? Or perhaps this is a major version bump? |
Another possibility is to make this "forward compatible" by using |
@BlakeWilliams I had proposed a major version bump back last time - is this the point we make the cutoff? |
Since we're below 1.0, we can technically make breaking changes at any time. Although I'd prefer we don't make breaking changes when we can avoid them. I think the best way forward is supporting both approaches since I think some bots will currently rely on the old behavior still being present. But we could recommend the new approach over the old one in the README and documentation. If we want to move towards a 1.0 release, I think an issue tracking some of the required work would be great 👍 |
Hey guys, any movement on this? The library currently doesn't work for any bots/apps created after November 2021, as the Maybe the lib could attempt to connect to |
FYI, slack has stated:
|
Is there a blocker for this PR to get upstreamed? |
This PR makes a few changes to adapt to Slack API changes...
handle_connect
callback to enable initializing both theslack
andprocess_state
rtm.connect
instead ofrtm.start
API:atom
keys since that's what everything inside this library already expectsSome docs changes are likely in order, and can be added...
fixes #233