Skip to content
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

broadcast_update_to partial should be smarter and extract the inner_html, it feels like a bug #644

Open
krschacht opened this issue Jul 6, 2024 · 0 comments

Comments

@krschacht
Copy link

krschacht commented Jul 6, 2024

I recently ran into a situation which feels like a bug with broadcast_update_to. After digging into this, I think it's working as intended, but I think it's so confusing as-is that it's effectively a bug we should consider fixing. If I can get acknowledgment this is worth fixing from maintainers, I'm happy to help on a PR if you'd like. Here's the situation:

I was doing a simple broadcast replace with a rails model partial. My basic setup was:

message.broadcast_replace_to message.conversation

And the front-end receives:

<div id="message_2992">
  Hello
</div>

But then I added a stimulus controller on every message div. While debugging, I added a console.log to the connection() method and I noticed every time a new message replace would stream in, a new instance of the stimulus controller would connect.

This is the intended behavior, but I need the same stimulus controller instance to persist across updates. Great, we have update for this. Easy change, right? I changed the key line to this and expected it to work:

message.broadcast_update_to message.conversation

The docs explicitly say, "Any handlers bound to the element would be retained. This is to be contrasted with the "replace" action above, where that action would necessitate the rebuilding of handlers."

However, the resulting front-end change is:

<div id="message_2992">
  <div id="message_2992">
    Hello there
  </div>
</div>

It is indeed using innerHTML in the replacement but not in the contents of what it's replacing!

If someone is using broadcast_update_to and explicitly specifying target and html, then there is no problem. But in the case where someone is using broadcast_update_to and simply specifying a model I think rails should do the smart thing and just pass in the inner_html of the partial.

This way all future engineers can easily change broadcast_replace_to to broadcast_update_to and if you're using rails model partials the resulting stream action will change from:

<turbo-stream action="replace" target="message_2992">
  <template>
     <div id="message_2992">
       Hello there
      </div>
  </template>
</turbo-stream>

To...

<turbo-stream action="update" target="message_2992">
  <template>
       Hello there
  </template>
</turbo-stream>

Does that make sense? This is the kind of conceptual compression we want to provide when you're following the rails conventions. Just to make this example more real-world, here is the actual helper method I wrote in within my worker to address this. It's a gross solution to address this surprising behavior of broadcast_update_to. I'm using Nokogiri to effective do element.inner_html of my partial:

  def self.broadcast_updated_message(message, locals = {})
    html = ApplicationController.render(
      partial: 'messages/message',
      locals: {
        message: message,
        message_counter: message.index
      }.merge(locals)
    )
    dom = Nokogiri::HTML.fragment(html)
    target = dom.at_css(':root').attributes["id"]
    html = dom.at_css(':root').inner_html
    message.broadcast_update_to message.conversation, target: target, html: html
  end
@krschacht krschacht changed the title I think broadcast_update_to partial should be smarter and extract the inner_html broadcast_update_to partial should be smarter and extract the inner_html, it feels like a bug Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant