Failing test case using image_tag and tag slot

[hchtlz]

I'm attempting to address an unexpected test failure that occurred while using the renders_one :tag feature in my component. The issue seems to be connected to the use of the image_tag method within the same component. The specific error message I encountered is:

Error:
SlotableTest#test_slotable_component:
ArgumentError: wrong number of arguments (given 2, expected 0)
    /view_component/lib/view_component/slotable.rb:91:in `block in renders_one'
    /.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/actionview-7.0.8/lib/action_view/helpers/asset_tag_helper.rb:413:in `image_tag'

I suspect that the issue may be related to the naming conflict between renders_one :tag and the image_tag method within the same component. When I changed the name "tag" to something different, the test case ran without any issues.

[Spone]

Thanks @hchtlz for opening this!

I think I understand what happens here:

• We create a slot named tag, so the tag method is defined in the component.
• When we call image_tag, the tag helper from Rails is called here... with 2 arguments, whereas the slot tag method doesn't expect any.

Now... I'm not sure how we want to address this:

• raise an error when you try to define a slot that has the same name as a Rails' helper? (but this would mean a lot of reserved names)
• only add tag to the list of reserved names (it's a pretty generic name so there are more chances of collision with this helper than, say, image_tag)
• figure out a way to prevent the collision (maybe related to the idea of allow-listing the helpers, see Replace helpers with explicit api in v4 #1848)

[Spone]

What would be your preferred approach for handling this @camertron @BlakeWilliams @boardfish?

[camertron]

@Spone what if we just raised an error if you tried to define a slot with the same name as an already-defined method? We actually already do this for polymorphic slot setters.

[Spone]

Sounds good. I'll work with @hchtlz on this.

[Spone]

Let's see if we need to do that for renders_many as well.

[Spone]

The failure in the Primer ViewComponents check comes from this component in Lookbook: https://github.com/ViewComponent/lookbook/blob/main/app/components/lookbook/icon_button/component.rb#L3C5-L3C51

We're calling renders_many :icons, which raises an exception since in this PR, we no longer allow slot names to overwrite existing method names.

But actually, I'm not sure whether we need this check for renders_many slots, since the singular slot name is never defined as a method by ViewComponent, right? So it would be legal to call both renders_many :icons and attr_reader :icon as you do in this component.

@allmarkedup mind chiming in?

[Spone]

Currently, validate_singular_slot_name is called on both renders_one and renders_manyslots.

view_component/lib/view_component/slotable.rb

Line 153 in b11ba1f

validate_singular_slot_name(ActiveSupport::Inflector.singularize(slot_name).to_sym)

The check for plural slot name conflicting with singular slot name has been introduced in #1455, but I'm not sure if I understand why that would be an issue (aside from being very confusing when you read the component).