-
Notifications
You must be signed in to change notification settings - Fork 330
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
Support renderable object as turbo stream content #433
Support renderable object as turbo stream content #433
Conversation
Add support for rendering renderable objects as turbo stream content. Renderable objects respond to `render_in(view_context, &block)`, returning an html_safe String (ActiveSupport::SafeBuffer).
@@ -227,6 +227,8 @@ def action_all(name, targets, content = nil, allow_inferred_rendering: true, **r | |||
private | |||
def render_template(target, content = nil, allow_inferred_rendering: true, **rendering, &block) | |||
case | |||
when content.respond_to?(:render_in) | |||
content.render_in(@view_context, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line could instead be @view_context.render(content, &block)
, which would give more control to the view context but will be slightly slower because of the extra indirection.
We’re already coupled to the render_in
method based on the respond_to?
check in the previous line, though that too could be delegated to the view context if the view context exposes a renderable-checking method.
I personally think this coupling with the render_in
interface is fine, but open to your input and wanted to highlight an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to delegate to Action View as much as possible, going as far as invoking render
when we know that it'll call render_in
behind the scenes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. The reason I didn’t do that was because we’re already coupled to render_in
here via the respond_to?
check. Shall I update it to this?
when content.respond_to?(:render_in)
@view_context.render(content, &block)
I don’t think we can do something like this without changes to ActionView.
when @view_context.renderable?(content)
@view_context.render(content, &block)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking this one on!
👋🏻 @seanpdoyle do you know who would be best to ping for a review to get this PR in? I'd love to see this land for folks using ViewComponent and Turbo like us at GitHub 😄 |
@afcapel @kevinmcconnell, maybe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @joeldrapper!
And thanks for reviewing, @seanpdoyle.
Add support for renderable objects as turbo stream content. Renderable objects respond to
render_in(view_context, &block)
, returning an html-safe String (anActiveSupport::SafeBuffer
).Example
This will improve compatibility with Phlex, ViewComponent, and anything else that uses the renderable duck type to provide object-oriented views.
Approach
I added a case to
render_template
in theTagBuilder
for when thecontent
argument is a renderable, based on whether it responds torender_in
.I’m not sure if this was the right testing strategy — I decided to make a new example resource ("notifications") rather than modify one of the existing examples. Please let me know if you have any suggestions on how to better approach that.