Accept symbol, proc, and nil arguments for broadcasts_refreshes

[mrrooijen]

Currently, broadcasts_refreshes only accepts values in the form of a string or nil, where explicitly passing nil bypasses broadcasting but still triggers the after_create_commit callback.

This change allows you to pass a symbol (method name to call) or a proc to evaluate to broadcasts_refreshes to produce a stream name dynamically at the instance level after_create_commit.

This allows us to, for example, specify a parent (Board) to broadcast refreshes to on create, while broadcasting to itself (Column) on update and destroy:

class Column < ApplicationRecord
  belongs_to :board
  broadcasts_refreshes :board
end

Listen for new Column creations for @board:

<%= turbo_stream_from @board %> 

Listen for updates and deletion of existing @column:

<%= turbo_stream_from @column %> 

This changes also disables the entire after_create_commit hook if nil is passed in explicitly, rather than allowing it to run without broadcasting anything.

[seanpdoyle]

where explicitly passing nil bypasses broadcasting but still triggers the after_create_commit callback.

What is the intended behavior of supporting explicitly nil arguments? To me, invoking the after_create_commit seems to be an accidental side-effect.

Rather than suffixing the if stream conditional, would an ArgumentError make sense?

Suggested change

	after_create_commit do
	case stream
	when String then broadcast_refresh_later_to(stream)
	when Symbol then broadcast_refresh_later_to(send(stream))
	else broadcast_refresh_later_to(stream.try(:call, self))
	end
	end if stream
	raise ArgumentError, "stream is required…" if stream.nil?
	after_create_commit do
	case stream
	when String then broadcast_refresh_later_to(stream)
	when Symbol then broadcast_refresh_later_to(send(stream))
	else broadcast_refresh_later_to(stream.try(:call, self))
	end
	end

[seanpdoyle]

Since this case statement aims to coerce the argument, but does not invoke different methods across the branches, what do you think about re-structuring it to extract out the method call?

Suggested change

	case stream
	when String then broadcast_refresh_later_to(stream)
	when Symbol then broadcast_refresh_later_to(send(stream))
	else broadcast_refresh_later_to(stream.try(:call, self))
	stream = send(stream) if Symbol === stream
	stream = stream.call(self) if stream.respond_to?(:call)
	broadcast_refresh_later_to(stream)

[mrrooijen]

Thanks for your input @seanpdoyle.

The intention of nil was to maintain backwards compatibility, but also to maintain the ability to skip the create hook by not creating it at all, and therefore not broadcasting messages that no one will ever listen for.

If I could break backwards compatibility, I'd probably change the API to the following, which disables the create hook by default, making it optional:

broadcasts_refreshes(stream = nil)

I think that, in the vast majority of cases, you want to either:

1. Skip the create broadcast entirely; or
2. Broadcast the creation to the parent or other related model.

Case for skipping: A Team model would result in a refresh broadcast to the teams stream name, which no one listens to. If no one listens to it, then why broadcast at all?

Case for broadcasting to related resources: A Board has many Columns, so if a new Column is created for a given Board, it's nice to be able to call broadcasts_refreshes :board on Column to notify any @board listeners of the creation of this Board's new Column.

The current default stream name is a static string based on the current model's pluralized name, and it does have use cases, but it seems far less common than either skipping or, for example, broadcasting to a parent.

Supporting Symbol and Proc are just quality-of-life improvements, and the ability to skip the create hook to avoid unnecessary broadcasts is just an optimization.

Ideally (in my opinion):

broadcasts_refreshes # only register update/destroy hooks, skip create hook by defaulting the stream argument to nil
broadcasts_refreshes :board # broadcast create to Board parent of current Column model, in addition to update/destroy hooks

Perhaps less ideal from an API design POV, but with backwards compatibility:

broadcasts_refreshes skip: :create # skip create hook despite `stream` being set to `model_name.pluralize` by default
broadcasts_refreshes :board # broadcast create to Board parent of current Column model, in addition to update/destroy hooks

If skipping won't be supported for this method, then, worst case, it results in a broadcast that no one will ever listen to, which isn't the end of the world, albeit suboptimal.