Skip to content
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

Action Cable: move channel_name to Channel.broadcasting_for #35021

Merged
merged 4 commits into from Jan 24, 2019

Conversation

@palkan
Copy link
Contributor

@palkan palkan commented Jan 22, 2019

Summary

That main purpose of this PR is to make the following example testable:

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:

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 (and how should developers know that it's even used?).

With this commit we could re-write the test as following:

 assert_broadcasts(ChatChannel.broadcasting_for(room), 1) do
   ChatRelayJob.perform_now
 end

IMO, this refactoring also makes the broadcasting_for method much easier to use internally, since we should't think about adding a channel_name every time we use it (like we did in channel/streams).

This PR also adds Channel#broadcast_to (we had #steam_for but hadn't a shorter syntax for broadcasting for some reason).

NOTE: in action-cable-testing we handled this differently: by adding channel: ... option to assertions; but that was a temporary hack since we couldn't change the Rails API.


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:
Copy link
Contributor

@georgeclaghorn georgeclaghorn Jan 22, 2019

If this method is intended to be used in app tests, it should be documented.

Copy link
Contributor Author

@palkan palkan Jan 22, 2019

Done.
Added a note to the change log.

stringify_broadcasting([ channel_name, model ])
end

def stringify_broadcasting(object)
Copy link
Contributor

@georgeclaghorn georgeclaghorn Jan 22, 2019

This method should be #:nodoc:ed or made private—it doesn’t seem to be used elsewhere, so preferably the latter.

Copy link
Contributor Author

@palkan palkan Jan 22, 2019

Done.

Copy link
Member

@kaspth kaspth Jan 22, 2019

Let’s also rename to serialize_broadcasting. Stringify sounds JS-like.

Copy link
Contributor Author

@palkan palkan Jan 22, 2019

Sure.

Stringify sounds JS-like.

Yep) I didn't like it either)

Thanks!

@palkan palkan force-pushed the refactor/broadcasting-for-testing branch from 03b8b18 to cd779c2 Jan 22, 2019

* Make `Channel::Base.broadcasting_for` a public API.

This change introduces a breaking change to internal Action Cable API:
Copy link
Member

@kaspth kaspth Jan 22, 2019

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.
Copy link
Member

@kaspth kaspth Jan 22, 2019

Now that this is public API, can we show an example?

palkan added 2 commits Jan 22, 2019
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 palkan force-pushed the refactor/broadcasting-for-testing branch from cd779c2 to a7d61f6 Jan 22, 2019
test "broadcast message to room" do
room = rooms[:all]
assert_broadcast_on(ChatChannel.broadcasting_for(room), text: "Hi!") do
Copy link
Member

@kaspth kaspth Jan 22, 2019

To be honest should we even expose broadcasting_for or just have this assertion call it automatically when passed a model?

Copy link
Contributor Author

@palkan palkan Jan 22, 2019

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.

include ActionCable::TestHelper
test "broadcast message to room" do
room = rooms[:all]
Copy link
Member

@kaspth kaspth Jan 22, 2019

Fixtures are a method call, not a hash lookup. Use parents instead of brackets.

Copy link
Contributor Author

@palkan palkan Jan 22, 2019

Oops!
Fixed in other related docs/guides too.

@palkan palkan force-pushed the refactor/broadcasting-for-testing branch from a7d61f6 to 513dd2c Jan 22, 2019
@kaspth kaspth merged commit 36c8400 into rails:master Jan 24, 2019
2 checks passed
@kaspth
Copy link
Member

@kaspth kaspth commented Jan 24, 2019

Thanks! I just realised I should have asked you to squash your commits down to 1, oh well, next time :D

@palkan palkan deleted the refactor/broadcasting-for-testing branch Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants