Action Cable: move channel_name to Channel.broadcasting_for #35021
Conversation
|
|
||
| module ClassMethods | ||
| # Broadcast a hash to a unique broadcasting for this <tt>model</tt> in this channel. | ||
| def broadcast_to(model, message) | ||
| ActionCable.server.broadcast(broadcasting_for([ channel_name, model ]), message) | ||
| ActionCable.server.broadcast(broadcasting_for(model), message) | ||
| end | ||
|
|
||
| def broadcasting_for(model) #:nodoc: |
There was a problem hiding this comment.
If this method is intended to be used in app tests, it should be documented.
There was a problem hiding this comment.
Done.
Added a note to the change log.
| stringify_broadcasting([ channel_name, model ]) | ||
| end | ||
|
|
||
| def stringify_broadcasting(object) |
There was a problem hiding this comment.
This method should be #:nodoc:ed or made private—it doesn’t seem to be used elsewhere, so preferably the latter.
There was a problem hiding this comment.
Let’s also rename to serialize_broadcasting. Stringify sounds JS-like.
There was a problem hiding this comment.
Sure.
Stringify sounds JS-like.
Yep) I didn't like it either)
Thanks!
palkan
force-pushed
the
refactor/broadcasting-for-testing
branch
from
03b8b18
to
cd779c2
Compare
actioncable/CHANGELOG.md
Outdated
|
|
||
| * Make `Channel::Base.broadcasting_for` a public API. | ||
|
|
||
| This change introduces a breaking change to internal Action Cable API: |
There was a problem hiding this comment.
A change log entry is only for public API. Just remove this “before”.
| end | ||
|
|
||
| def broadcasting_for(model) #:nodoc: | ||
| # Returns a unique broadcasting identifier for this <tt>model</tt> in this channel. |
There was a problem hiding this comment.
Now that this is public API, can we show an example?
That would allow us to test broadcasting made with channel, e.g.:
```ruby
class ChatRelayJob < ApplicationJob
def perform_later(room, msg)
ChatChannel.broadcast_to room, message: msg
end
end
```
To test this functionality we need to know the underlying stream name
(to use `assert_broadcasts`), which relies on `channel_name`.
We had to use the following code:
```ruby
assert_broadcasts(ChatChannel.broadcasting_for([ChatChannel.channel_name, room]), 1) do
ChatRelayJob.perform_now
end
```
The problem with this approach is that we use _internal_ API (we shouldn't care about `channel_name` prefix
in our code).
With this commit we could re-write the test as following:
```ruby
assert_broadcasts(ChatChannel.broadcasting_for(room), 1) do
ChatRelayJob.perform_now
end
```
palkan
force-pushed
the
refactor/broadcasting-for-testing
branch
from
cd779c2
to
a7d61f6
Compare
| test "broadcast message to room" do | ||
| room = rooms[:all] | ||
|
|
||
| assert_broadcast_on(ChatChannel.broadcasting_for(room), text: "Hi!") do |
There was a problem hiding this comment.
To be honest should we even expose broadcasting_for or just have this assertion call it automatically when passed a model?
There was a problem hiding this comment.
We do this automatically when we're within a channel test case (has been implemented in #33969), but we cannot do this in other contexts, 'cause we don't know about the channel.
guides/source/testing.md
Outdated
| include ActionCable::TestHelper | ||
|
|
||
| test "broadcast message to room" do | ||
| room = rooms[:all] |
There was a problem hiding this comment.
Fixtures are a method call, not a hash lookup. Use parents instead of brackets.
There was a problem hiding this comment.
Oops!
Fixed in other related docs/guides too.
palkan
force-pushed
the
refactor/broadcasting-for-testing
branch
from
a7d61f6
to
513dd2c
Compare
|
Thanks! I just realised I should have asked you to squash your commits down to 1, oh well, next time :D |
Summary
That main purpose of this PR is to make the following example testable:
To test this functionality we need to know the underlying stream name
(to use
assert_broadcasts), which relies onchannel_name.We had to use the following code:
The problem with this approach is that we use internal API: we shouldn't care about
channel_nameprefix in our code (and how should developers know that it's even used?).With this commit we could re-write the test as following:
IMO, this refactoring also makes the
broadcasting_formethod much easier to use internally, since we should't think about adding achannel_nameevery time we use it (like we did inchannel/streams).This PR also adds
Channel#broadcast_to(we had#steam_forbut hadn't a shorter syntax for broadcasting for some reason).NOTE: in
action-cable-testingwe handled this differently: by addingchannel: ...option to assertions; but that was a temporary hack since we couldn't change the Rails API.