turbo_frame_tag can no longer accept an array of ids

[jwilsjustin]

In version 1.4.0 the turbo_frame_tag helper method accepted one or more ids or records.

this:

<%= turbo_frame_tag customer, field do %>
  <!-- code -->
<% end %>

would produce this:

<turbo-frame id="customer_1_field_1">
  <!-- code -->
</turbo-frame>

There appears to be an issue in 1.5.0 where passing more than one id or record doesn't behave as expected.

this:

<%= turbo_frame_tag customer, field do %>
  <!-- code -->
<% end %>

is producing:

<turbo-frame id="#<Field:0x00000001179645c0>_customer_1">
  <!-- code -->
</turbo-frame>

The regression appears to have been introduced in #476. The list of ids is getting "splatted" into the ActionView::RecordIdentifier.dom_id method: https://github.com/hotwired/turbo-rails/pull/476/files#diff-7694cf5e712468388f0d32c6638e9d9508b612f0346ce4beb9093960a757fa59R39

But ActionView::RecordIdentifier.dom_id does not accept a collection: https://api.rubyonrails.org/classes/ActionView/RecordIdentifier.html#method-i-dom_id

[jwilsjustin]

I see now in #476 it says:

This does break the use case of passing multiple models, but that seems like it would be better fixed in dom_id itself.

So I suppose the plan is to enhance ActionView::RecordIdentifier.dom_id to be "splat-able".

[seanpdoyle]

Thank you for exploring this @jwilsjustin!

So I suppose the plan is to enhance ActionView::RecordIdentifier.dom_id to be "splat-able".

@skipkayhil as the author of #476, I'm curious how you feel about rails/rails#44081

[jwilsjustin]

Thanks for responding @seanpdoyle. I don't know if rails/rails#44081 would fix this particular issue though since the dom_id method would still accept only one record (from what I can tell in that PR). But that PR could certainly serve as a starting point.

[skipkayhil]

@skipkayhil as the author of #476, I'm curious how you feel about rails/rails#44081

👍 Based on Kasper's feedback it seems like the missing piece was a concrete need to make a change to dom_id, and it seems like id generation for Turbo Frames may be just the thing.

I don't know if rails/rails#44081 would fix this particular issue though since the dom_id method would still accept only one record (from what I can tell in that PR). But that PR could certainly serve as a starting point.

Right, to support multiple records passed to dom_id, it would need to be additionally changed to try to get the key for every single prefix.

[jwilsjustin]

@skipkayhil @seanpdoyle AFAIK new features like this (supporting multiple models passed to dom_id) are not supposed to be suggested via GitHub issues in rails/rails. Could either of you direct me to where I should make such a suggestion? Or is an issue on the rails/rails repo the correct place to suggest this?

[skipkayhil]

@skipkayhil @seanpdoyle AFAIK new features like this (supporting multiple models passed to dom_id) are not supposed to be suggested via GitHub issues in rails/rails. Could either of you direct me to where I should make such a suggestion? Or is an issue on the rails/rails repo the correct place to suggest this?

If you want to discuss whether the change would be accepted, https://discuss.rubyonrails.org would be the place to go. Other than that there is not really any home for feature suggestion (by design). Instead of discuss, you can also just open a PR with the feature implemented.

[jwilsjustin]

@skipkayhil Roger that. Thanks! I started a discussion. I'm going to see if I can get a PR going. Thank you for your helpful comments 💪

[jwilsjustin]

FWIW I made a PR that will allow us to pass an array of records or classes to the dom_id method: rails/rails#50608.

When or if that gets merged, I'm happy to make a PR here to make the necessary changes to support arrays passed to turbo_frame_tag.