slack-ruby / slack-ruby-client

A Ruby and command-line client for the Slack Web, Real Time Messaging and Event APIs.
MIT License
1.21k stars 214 forks source link

Initialize store data on successful connection (hello) #423

Closed kstole closed 1 year ago

kstole commented 1 year ago

This is based on the fix my team implemented in our bot to replace the old rtm.start behavior. I debated where this belongs and open to feedback. One option would be to put it into Slack::RealTime::Client#build_socket and Slack::RealTime::Client#restart_async, which could retrieve data from the Web API before or after connecting to the RTM API, but it seemed cleaner triggering it on the successful hello message. It could also be a standalone built-in handler (that's how we implemented in our bot), but it makes a lot of sense belonging to the store and automatically avoids conflicting with the Starter store. Finally, I considered an option to disable this initialization functionality, but that's essentially possible by having it in Slack::RealTime::Store and not Slack::RealTime::Stores::Store (detailed in README and UPGRADING).

dblock commented 1 year ago

I am a fan of doing less by default and letting users do expensive things like enumerating all users knowingly. I can't imagine how long users_list or conversations_list is going to take on a large corporate slack. The reason why rtm.start was deprecated is precisely for the same reason why we should deprecate the initialization behavior here as we bump to a major 2.0.

So I am thinking we should do this:

  1. By default I would keep Slack::RealTime::Store as is.
  2. Document in UPGRADING.md that since https://github.com/slack-ruby/slack-ruby-client/pull/419/files, and because of the rtm.start change, the store is no longer initialized with data.
  3. Introduce a new store, Slack::RealTime::Auto that initializes channels, users, etc.
  4. [Optional] Make Slack::RealTime::Auto configurable for what it is capable of enumerating (channels vs. users).
  5. Document in UPGRADING.md to use Slack::RealTime::Auto to obtain previous rtm.start behavior, and document differences.

WDYT?

kstole commented 1 year ago

My thought was trying to keep the previous default behavior of the gem (using rtm.start and Slack::RealTime::Store fully loaded) and I think this is closest to the previous behavior (at least it paginates the data loading). There's a difference, though, that setting start_method was a way to change the store class (if you chose rtm_connect, you got the Starter store by default). Now that rtm_connect is the only start method, it seems strange to use the full Store that reacts to all changes if it doesn't have that data loaded.

I'm fine with creating a new store that houses this additional initialization but then I think Starter should be the default store. And at that point, what are we doing with Slack::RealTime::Store?

dblock commented 1 year ago

My thought was trying to keep the previous default behavior of the gem (using rtm.start and Slack::RealTime::Store fully loaded) and I think this is closest to the previous behavior (at least it paginates the data loading).

Yes, I understand. But this is the time to tell people to stop doing that since that kind of behavior was deprecated. You're offering a path back, but it shouldn't be the default as a major breaking change because it's bad (TM).

There's a difference, though, that setting start_method was a way to change the store class (if you chose rtm_connect, you got the Starter store by default). Now that rtm_connect is the only start method, it seems strange to use the full Store that reacts to all changes if it doesn't have that data loaded. I'm fine with creating a new store that houses this additional initialization but then I think Starter should be the default store. And at that point, what are we doing with Slack::RealTime::Store?

Or should we keep Slack::RealTime::Store as the default store that does nothing? Or default to nil?

We can probably just remove Slack::RealTime::Store?