[FrancescoK]

Fixes #2715.

turbo_frame_tag and image_tag choose how to build their implicit attribute from
the value's class, at render time:

id = ids.first.respond_to?(:to_key) || ids.first.is_a?(Class) ? dom_id(*ids) : ids.join('_')

source.is_a?(Symbol) || source.is_a?(String) ? path_to_asset(source, ...) : polymorphic_url(source)

Resolving them into markup chose statically, from the shape of the argument, so a local holding a String reached dom_id and raised NoMethodError: undefined method 'to_key' for an instance of String, and an Active Storage blob reached image_path and resolved to "".

The type does not have to be known at compile time. Both now emit the branch the helper itself performs:

<%= turbo_frame_tag frame_id do %>Content<% end %>

_buf << ::Herb::Engine.attr(
  (frame_id).then { |value| value.respond_to?(:to_key) || value.is_a?(Class) ? dom_id(value) : value.to_s }
)

The argument binds once through then, so an expression with side effects or a query still evaluates a single time.

What is unchanged

A literal argument still folds to static markup — turbo_frame_tag "tray" still
compiles to <turbo-frame id="tray"> with nothing left to evaluate — and a literal dom_id(...) call is still passed through. Only the arguments that previously got a guess now carry the branch, and those were calling the whole helper before the resolution existed.

skip_wrapping_for in config/action_view_helpers/**.yml is untouched. It still decides what can fold; it is no longer what decides correctness.

Multiple ids

turbo_frame_tag joins or dom_ids every positional id, not just the first, so turbo_frame_tag record, "notes" was dropping the prefix (reported by @mattboldt below). Literal ids now join at compile time (turbo_frame_tag "notes", :list gives notes_list). Anything else carries the same branch the helper performs, over all ids:

["notes", frame_id].then { |ids| ids.first.respond_to?(:to_key) || ids.first.is_a?(Class) ? dom_id(*ids) : ids.join("_") }

Two engine tests cover it.

Tests

turbo_frame_tag with a string id held in a local is added to the engine tests,
which render through Action View and compare. It fails without this change and the rendered output matches Rails with it:

<turbo-frame id="post_1">Content</turbo-frame>

Three existing snapshots move from the guess to the branch. Their subjects — a model and an instance variable — still resolve through dom_id at render time, so the behaviour they describe is unchanged.

rake test passes (5203 runs, 13718 assertions, 0 failures)

[FrancescoK]

CI caught a package I had not run: @herb-tools/rewriter. Pushed a fix, but:

The rewriter reads the same resolved attribute as the compiler, so changing the resolution changes what the codemod writes into a template:

<%= image_tag @post.cover_image %>

<img src="<%= (@post.cover_image).then { |value| value.is_a?(String) || value.is_a?(Symbol) ? image_path(value) : polymorphic_url(value) } %>" />

That might be correct, but definitely unpleasant to read. The expectation it replaces was image_path(@post.cover_image), which is concise yet wrong in exactly the way this
PR is about (an Active Storage source resolves to "") so the codemod was writing the bug into people's templates.

[mattboldt]

We hit #2715 with ReActionView slots on, and this fixes it for us. I tested it cherry-picked onto v0.11.1 (applies cleanly): every turbo_frame_tag that was raising undefined method 'to_key' now renders with the right id, and nothing else broke.

One small thing for a follow-up: turbo_frame_tag record, "notes" still drops the "notes" prefix, since only the first argument gets read. That was already the case before this PR.

Thanks!

[FrancescoK]

Thanks for testing this, @mattboldt. I added the multi-id case to this PR, so turbo_frame_tag record, "notes" now keeps the "notes" prefix the way Rails does. It is a new commit on top of what you cherry-picked, so if you get a chance to re-test, that would be great.