Decouple ActionCable application logic from low-level logic#27648
Decouple ActionCable application logic from low-level logic#27648palkan wants to merge 4 commits intorails:masterfrom
Conversation
7d765e5 to
51e2e54
Compare
maclover7
left a comment
There was a problem hiding this comment.
I love the idea behind this PR, and I think this is a really, really good start at separating out some of the concerns with ActionCable::Connection. Besides for one concern, this PR looks amazing!
My fear is that we are introducing too many top-level classes for users to keep track of. If we merge in this PR, then we'll have three: Connection, Client, and Channel. IMHO, Client should be "internal" / "low-level / close to the metal" API, and should not be public for users to be interacting with.
It's possible we might not need to generate Connection anymore based on this PR, but that also brings up the issue of backwards compatibility with Rails 5 apps that already have #connect methods (just as an example) declared...
Thank you again so much for working on this, @palkan!!
There was a problem hiding this comment.
Should we keep connection_class around, and introduce a new config option called client_class or similar? Otherwise in ActionCable::Server::Base#call we are declaring an explicit dependency on ActionCable::Connection::Base.
There was a problem hiding this comment.
Is there a way to keep this type of code inside of ActionCable::Connection::Base? Otherwise, when, for example, generating a new Rails application we'll have to create three ApplicationCable files: Connection, Client, and Channel...
Let me clarify: this PR replaces Connection (which is internal) with Client. Which is definitely a bad idea( We need a better name for "internal" entity and use "Connection" for the "public" one (Client). What about "Socket"? Connection remains the same from the API point of view, Socket is a layer between raw WebSocket and public Connection. |
|
@palkan totally agree. We need to tease out a "public" facing |
af59573 to
94df353
Compare
Done! Now it's backward-compatible. |
|
Is full backwards compatibility ensured? |
|
@matthewd I'm deferring this to after 5.1. If you want to include it, let me know. |
94df353 to
76bf81f
Compare
Integration tests are passed (without any change). I've also checked my existent apps (e.g. this one I use for Action Cable experiments) – everything works fine. |
f9ce7fe to
da8962d
Compare
da8962d to
d1c330d
Compare
|
I just saw that there is some work ongoing for ActionCable regarding testing for Rails 6. I was wondering if some of the other ActionCable PRs by you are also to be revisited for Rails 6? |
I would be glad to revisit them too; and propose even more PRs later) I think, major release is a great chance to revisit the whole framework, to discuss its weak or missing parts and fix them. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This PR is a continuation of the following conversation:
Me:
@dhh
So, here is it)
What has been done:
Connectionhas been split up intoConnection(socket wrapper, low-level) andClient(although I don't like the name, app-level).Client itself still depends on
serverand its event loop, but also can be isolated.What's this refactoring for?
This will allow us to completely separate ActionCable public API from low-level sockets manipulations and thus will make it possible to use, for example, different server implementations with ease.
It will also simplify ActionCable channels unit-testing (#27191) and self-testing too.
P.S. "Refactor ActionCable streaming" PR (#27044) can be a part of this process.