Allow passing a :template option to Broadcastable methods
#425
Conversation
TL;DR: This PR allows passing the `:template` option to the `**rendering` options of a `Broadcastable` method, so we can render a whole template instead of a partial. Rationale: Currently, we can only use :partial, :html or pass nothing (in this case `Broadcastable` calls the model's `to_partial_path` method). Meaning we're assuming that we'll always be dealing with partials or custom HTML written inline. If we rather want to render a whole template (e.g. index, show, edit, etc.), we need to resort to using `ApplicationController.render`, which is not ideal.
| assert_broadcast_on "stream", turbo_stream_action_tag("update", target: "message_1", template: render("messages/index", layout: false)) do | ||
| @message.broadcast_update_to "stream", template: "messages/index" | ||
| end | ||
| end |
There was a problem hiding this comment.
I'm wondering if we'd want:
- Only one method tested with the
templateoption, like the current commit has. - Separate tests for each
Broadcastablemethod. e.g.
test "broadcasting update to stream now with template option" do
assert_broadcast_on "stream", turbo_stream_action_tag("update", target: "message_1", template: render("messages/index", layout: false)) do
@message.broadcast_update_to "stream", template: "messages/index"
end
end
test "broadcasting replace to stream now with template option" do
assert_broadcast_on "stream", turbo_stream_action_tag("replace", target: "message_1", template: render("messages/index", layout: false)) do
@message.broadcast_replace_to "stream", template: "messages/index"
end
end- Separate methods in a dynamic way, e.g.:
[
[:broadcast_update_to, :update],
[:broadcast_replace_to, :replace]
].each do |method, action|
test "broadcasting #{method} to stream now with template option" do
assert_broadcast_on "stream", turbo_stream_action_tag(action, target: "message_1", template: render("messages/index", layout: false)) do
@message.public_send method, "stream", template: "messages/index"
end
end
end
[
[:broadcast_before_to, :before],
[:broadcast_after_to, :after],
[:broadcast_append_to, :append],
[:broadcast_prepend_to, :prepend],
].each do |method, action|
test "broadcasting #{method} to stream now with template option" do
assert_broadcast_on "stream", turbo_stream_action_tag(action, target: "message_1", template: render("messages/index", layout: false)) do
@message.public_send method, "stream", template: "messages/index", target: "message_1"
end
end
end
# ...etc.Reason I only added one test for now is because the current tests are not thoroughly testing all possible rendering options for all methods. e.g. the partial option is only tested in the test named local variables don't get overwritten if they collide with the template name, so I don't know how to proceed.
There was a problem hiding this comment.
Prefer the separate tests. Please do open a follow up PR to expand test coverage!
| # def update_message | ||
| # broadcast_replace_to(user, :message, target: "message", template: "messages/show", locals: { message: self }) | ||
| # end | ||
| # end |
There was a problem hiding this comment.
This might not be the best example but I'm just trying to follow the existing "messages" context of the examples.
|
What would the use case be for broadcasting a view template? I would normally expect the view templates to be rendered as the response to a controller action. When you want to reuse content from a view in multiple scenarios, extracting that content to a partial is a nice convention. It also means you're passing locals rather than ivars to those partials, whereas the views will typically expect state in ivars. There's probably a good reason for this that I'm missing though! |
|
That's a valid question @kevinmcconnell! Personally, my use case is this: I have a turbo-frame modal that works as a stepper/wizard. It has a In this case, the
Having the option to use Now, aside from my use case. I think this change is also valid for 2 reasons:
Finally, regarding the use of locals rather than ivars. I personally always use locals/helper methods rather than ivars. Since if an ivar is not defined, Ruby will not complain, and so you might end up with However, since this these options are all going to be passed down to |
|
@dhh Failing CI tests pass on my local, e.g.
I've seen other PRs merged with some failing CI jobs, so not sure what the next steps are. |
|
Yeah CI is flaky. Will fix elsewhere. |
|
@davidalejandroaguilar thanks for the explanation! I'd still use a partial for that case myself (but not called
I think that's a great idea 👍 |
Description
Summary
This PR allows passing the
:templateoption to the**renderingoptions of aBroadcastablemethod, so we can render a whole template instead of a partial.Rationale
Currently, we can only use
:partial,:htmlor pass nothing (in this caseBroadcastablecalls the model'sto_partial_pathmethod). Meaning we're assuming that we'll always be dealing with partials or custom HTML written inline. If we rather want to render a whole template (e.g. index, show, edit, etc.), we need to resort to usingApplicationController.render, which is not ideal.Notes
When the
templateoption is passed, we'll pass it down with thelayout: falseoption, in order to avoid passing any unwanted elements such as<head>.