Pass a String to ActiveJob (rather than a SafeBuffer)

[michaelbaudino]

This provides compatibility with Sidekiq as an ActiveJob adapter (because Sidekiq only allows native JSON types to be passed as job arguments).

All credit goes to @jdelStrother who suggested this solution.

Fixes #522, fixes #535

[michaelbaudino]

By the way, @jdelStrother, I'd be happy to close this PR if you want to create another one to be the author of the commit.

[jdelStrother]

No worries, happy to leave you as the author

[Michoels]

I could really use this, are there plans for it to be merged?

[dhh]

Needs an explanation otherwise its a bit of a mystery method.

[Michoels]

Would an explanatory comment suffice?
Or would you prefer if it were wrapped in a descriptive method, like this:

def self.perform_later(stream, content:)
  super(stream, content: serialize_for_compatibility(content))
end

# Sidekiq requires job arguments to be valid JSON types, such as String
def serialize_for_compatibility safe_buffer
  safe_buffer.to_str
end

[dhh]

Actually, there's something that rubs my a bit wrong about this overwrite. Feels like we should just ensure that we're calling this perform_later method with the right type. Rather than leave it this late to do the conversion.

[Michoels]

In that case, the correct patch for this issue would be in Turbo::Streams::Broadcasts:

def broadcast_refresh_later_to(*streamables, request_id: Turbo.current_request_id, **opts)
  refresh_debouncer_for(*streamables, request_id: request_id).debounce do
    content = turbo_stream_refresh_tag(request_id: request_id, **opts)
    Turbo::Streams::BroadcastStreamJob.perform_later stream_name_from(streamables), content: serialize_for_compatibility(content)
  end
end

# Sidekiq requires job arguments to be valid JSON types, such as String
def serialize_for_compatibility safe_buffer
  safe_buffer.to_str
end

Or if we skip the descriptive method:

def broadcast_refresh_later_to(*streamables, request_id: Turbo.current_request_id, **opts)
  refresh_debouncer_for(*streamables, request_id: request_id).debounce do
    Turbo::Streams::BroadcastStreamJob.perform_later stream_name_from(streamables), content: turbo_stream_refresh_tag(request_id: request_id, **opts).to_str
  end
end

[dhh]

That's just the refresh methods? Wouldn't this be an issue for all the _later methods?

[Michoels]

The OP issue does not appear to affect the other *_later methods.

The underlying problem is that we're passing a rendered SafeBuffer to ActionJob, which is rejected by Sidekiq.

broadcast_refresh_later_to is the only method that does this.

The other methods do their rendering inside the job, so the SafeBuffer never hits the Sidekiq serializer.

(Sidebar: broadcast_refresh_later_to is rather different than the other methods. It's the only one that has debouncing, has request_id, and it enqueues a different job. Perhaps it should be extracted to a separate module? It seems like this system could be more streamlined and elegant)

Technical details

At the moment there are seven broadcast_*_later_to methods.

1. broadcast_refresh_later_to, directly renders the <turbo-stream> and enqueues a Turbo::Streams::BroadcastStreamJob, causing the OP problem. (Line 73)
2. The other 6 stream actions delegate to broadcast_action_later_to.
3. broadcast_action_later_to enqueues Turbo::Streams::ActionBroadcastJob, without rendering. (Lines 84-85)
4. Turbo::Streams::ActionBroadcastJob calls #broadcast_action_to
5. #broadcast_action_to renders the <turbo-stream> and directly broadcasts it, without an additional job.

The difference between the methods can be demonstrated in the Rails console by running:

user = User.first
user.broadcast_update_later
# => Enqueued Turbo::Streams::ActionBroadcastJob
user.broadcast_refresh_later
# => :Failed enqueuing Turbo::Streams::BroadcastStreamJob to Sidekiq(default): ArgumentError (Job arguments to Turbo::Streams::BroadcastStreamJob must be native JSON types, but "<turbo-stream action=\"refresh\"></turbo-stream>" is a ActiveSupport::SafeBuffer.

Bottom line:
Only broadcast_refresh_later_to is affected, and patching it should solve the OP issue.

[dhh]

Great. So let's just do a PR where we call to_str at the call site rather than overwrite the perform_later method.

[Michoels]

@michaelbaudino @jdelStrother would you like to author the new PR?
Otherwise I have one ready to go in PR 683.
(Using my main GitHub account)

[jdelStrother]

No that's fine. Think you've done most of the work in this PR 🙂

[michaelbaudino]

That's awesome thanks @Michoels for the investigation and the new PR 🙏

And thanks @dhh for the review (and please ignore this PR in favor of #683 👍)