[BREAKING CHANGE] Replace rtm.start with rtm.connect

[acconrad]

Replace rtm.start with rtm.connect and bump to a major version along with a migration guide.

[BlakeWilliams]

👋 thanks for tackling this. In terms of tests I've setup a basic fake API. The initial entry-point is here: https://github.com/BlakeWilliams/Elixir-Slack/blob/master/test/support/fake_slack.ex#L22 which then uses the Router defined here: https://github.com/BlakeWilliams/Elixir-Slack/blob/master/test/support/fake_slack/router.ex

It's a simple Plug based router/app so we could likely extend that to return fake results for each of those API Calls.

[acconrad]

@BlakeWilliams cool I'll check that out tonight.

Another option I considered is having 2 private functions within the init call:

1 that we currently have that supports the current functionality of the 5 requests going in tandem to legacy support rtm.start.

The other function would not have that flag on and would only return the standard rtm.connect base objects. That way we can send a deprecation message in the old function saying something like "pulling all objects will be retired in the next version, update accordingly" - and then when you update a major version of this plugin, all you have to do is delete the old function and rely only on the new function.

Does that make sense?

[BlakeWilliams]

That makes sense to me. As long as we expose a way to bootstrap each of the possible fields individually when using the 2nd option (as an argument or a function, I kinda lean towards a function) I think that'd work API wise.

[acconrad]

@BlakeWilliams so I looked into the call and the problem is that the init function is never called in your library. It's only called via websocket_client (the Erlang dependency), so there is no way for me to force in that argument (or alternative function).

Here is what I would recommend: we push this up right now so we can fix everyone's issue and bump the version so 0.16 -> 0.17. Yes, it requires a bunch more calls, but 5 HTTP calls is honestly a drop in the bucket and does solve the bug. At the next version 0.2 (or 1.0, whatever the next breaking bump) you just need to change the README to state that the slack state no longer returns everything that it originally did and probably include a migration guide (happy to provide that for you).

[BlakeWilliams]

@acconrad That sounds like a solid plan. I'm in favor of shipping with the extra HTTP calls now then removing them later as a breaking change (+ appropriate version bump).

[BlakeWilliams]

@acconrad I'd also really like to see a deprecation warning letting folks know that a future release will no longer auto-populate those fields with a link to the migration guide once we get that in. I think that'd be good UX for folks using the package.

[acconrad]

@BlakeWilliams all set! I also fixed the tests, added the deprecation, and bumped it by 1 minor version and provided the appropriate documentation

[acconrad]

@BlakeWilliams ready to merge when you are

[acconrad]

@BlakeWilliams just checking in - fixed conflicts, ready when you are

[acconrad]

If possible, please revert rtm.start commit and release a minor version bump

@Slotos after reading all of these comments I'm just going to close this PR and re-open it with a major version bump since it has breaking changes. I don't want to be the one who is releasing the actual V1 of this but I think it's necessary to expose the gravity of the upgrade required. Revert PR incoming.

[BlakeWilliams]

@acconrad Thanks a ton for all your work on this. I definitely think this is the way to go but I agree the path forward is a major release. 👍

[Slotos]

@acconrad thank you. I've left some responses in threads to share more info, I hope it comes in handy.

[acconrad]

@Slotos @BlakeWilliams @chulkilee

OKAY!

This should go in after #190 - first, we add in #190 to fix everyone's problems with rtm.connect. Then we introduce this fix to properly deprecate the Slack API while also providing a deprecation guide on how to upgrade and maintain current functionality (if you so desire all of those lists).

All tests are passing, there is no longer a dependency on the mock library since those tests are no longer necessary, and the code is just simpler overall.

Let me know what you think!

[acconrad]

190 just went in so I'll fix the conflicts tonight and this can be RTM

[acconrad]

@BlakeWilliams alright conflicts resolved - this should be good to go!

[acconrad]

@BlakeWilliams all conflicts resolved - can we get this merged in?

[acconrad]

@BlakeWilliams all merge conflicts resolved and all tests passed - version bumped down to 0.20 can we get this merged?

[BlakeWilliams]

Thanks!