Skip to content

Conversation

@davidalejandroaguilar
Copy link
Contributor

@davidalejandroaguilar davidalejandroaguilar commented Oct 13, 2023

Description

#364 added support for passing a renderable to Broadcastable methods. However, it only added it for the synchronous methods. The asynchronous (_later) methods are broken because the renderable instance is not serialized.

This PR serializes the renderable before enqueueing it.

Note that the above means that rendering will happen synchronously, while the broadcast will be done asynchronously. If we want to avoid sync rendering, we'd have to look for a way to properly serialize a renderable instead of rendering it beforehand.

Comment on lines +68 to +70
if rendering[:renderable]
rendering[:html] = rendering.delete(:renderable).render_in(self)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the property re-assignment necessary? The action_view code that checks for respond_to?(:render_in) seems to expect the object under the :renderable key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. View Components aren't serializable to Active Job arguments:

Error:
BroadcastsTest#test_Message_broadcast_prepend_later_with_renderable:_render_option:
ActiveJob::SerializationError: Unsupported argument type: MessageComponent
    /Users/seanpdoyle/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/activejob-6.1.7.2/lib/active_job/serializers.rb:30:in `serialize'
    /Users/seanpdoyle/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/activejob-6.1.7.2/lib/active_job/arguments.rb:122:in `serialize_argument'

This means that if we stick with the current proposed change, rendering of the renderable: option would occur immediately, in-process with the request, and the broadcast would be enqueued to a background job.

Prior to this change, the rendering itself occurs in the background job alongside the broadcasting. This change would mean that rendering: options are a special case, require special care, and could have adverse effects on the request-response cycle if the rendering is expensive.

I'm curious what it'd take to make ViewComponent ActiveJob-serializable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is.

Since we're rendering the renderable in advance, if we don't re-assign, the :renderable key will have a ActiveSupport::SafeBuffer value, and Turbo::Streams::Broadcasts#broadcast_action_to will call render_format(:html, **rendering) causing it to fail with:

NoMethodError: private method `format' called for "<div class='test-message'>Test message</div>":ActiveSupport::SafeBuffer

Because it's not actually a renderable that responds to format, but rendered HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was going to mention exactly that:

That what will happen in the background is the actual broadcasting, but the rendering will still be synchronous. Unless we somehow serialize the component in a way that is compatible with ActiveJob / Sidekiq

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this change, the rendering itself occurs in the background job alongside the broadcasting.

Well, not exactly true, prior to this change, these _later methods using a renderable are entirely broken 😄

@sbounmy
Copy link

sbounmy commented May 8, 2025

Following this, having the issue with _later and renderable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants