Thanks for opening this PR, I like the idea behind this change!

It looks like there may be some failures in the primer view component test suite though. I'll re-run them to verify they aren't just flaky.

I've been struggling to think of a good way to add a test for this that isn't a contrived assertion that it's not a child of Object.

I realized after looking through the test suite that I'm using slots a bit differently than maybe it's meant to be used. I am rarely using another ViewComponent as the slot class. Normally it is a basic Struct. So my use would be something like this:

class PageHeaderComponent < ViewComponent::Base
  renders_many :actions, "Action"
  
  Action = Struct.new(:title, :icon, :href, :method, :data, keyword_init: true) do
    def initialize(...)
      super
      self.data ||= {}
    end
  end
end

And then my template would look something like this:

<div class="page-header">
  <div class="page-heading-container">
    <h2 class="page-heading">
      <%= content %>
    </h2>
  </div>

  <% if actions.any? %>
    <div class="actions">
      <% actions.each do |action| %>
        <span class="hidden sm:block">
          <a type="button" href="<%= action.href %>" class="button" <%=raw tag.tag_options({data: {method: action.method, **action.data}}, true) %>>
            <% if action.icon.present? %>
              <i class="<%= action.icon %>"></i>
            <% end %>
            <%= action.title %>
          </a>
        </span>
      <% end %>

      <span class="ml-3 relative sm:hidden" data-controller="dropdown">
        <button data-action="click->dropdown#toggle" type="button" class="button">
          More
          <i class="fa fa-arrow-down"></i>
        </button>

        <div data-dropdown-target="menu" class="dropdown-menu">
          <% actions.each do |action| %>
            <a href="<%= action.href %>" class="dropdown-item" <%=raw tag.tag_options({data: {method: action.method, **action.data}}, true) %>>
              <%= action.title %>
            </a>
          <% end %>
        </div>
      </span>
    </div>
  <% end %>
</div>

So rather than the slots being components that I'm instantiating and rendering via their own template file, I'm using them as buckets of data (Structs in this case) and I'm handling the "rendering" of them in the parent component by accessing the fields on the struct. You can see in the template above I'm doing something that (as far as I am aware) wouldn't be possible if the slot was a ViewComponent: I'm rendering each action twice. Once as a button when you're viewing on a large screen, and then again inside of a dropdown menu that displays only on mobile. I don't think I can achieve that level of control if the action had its own template that I rendered.

And then just for completeness-sake, here's how I use that component:

<%= render Admin::PageHeaderComponent.new do |header| %>
  <% if @purchase_order.bottles.any? && @purchase_order.clearable? %>
    <% header.action title: "Clear All SKUs", href: clear_bottles_admin_purchase_order_path(@purchase_order), icon: "fa fa-bomb", method: :patch, data: { confirm: "Are you sure?", disable_with: "Deleting #{@purchase_order.bottles.count} SKUs..."} %>
  <% end %>
  <% header.action title: "Activate All", href: enable_bottles_admin_purchase_order_path(@purchase_order), icon: "fa fa-store" %>
  <% header.action title: "Deactivate All", href: disable_bottles_admin_purchase_order_path(@purchase_order), icon: "fa fa-store-slash" %>
  <% header.action title: "Export", href: csv_export_admin_purchase_order_path(@purchase_order, format: :csv), icon: "fa fa-file-export" %>
  <% header.action title: "Sell Rate", href: sell_rate_admin_purchase_order_path(@purchase_order), icon: "fa fa-chart-line" %>
  <% header.action title: "Edit", href: edit_admin_purchase_order_path(@purchase_order), icon: "fa fa-pencil", primary: true %>
  PO ##{@purchase_order.id}
<% end %>

So I bring all of this up to say that the usefulness of this PR, I think, only comes into play if you're going to be calling methods on the Slot from the parent's template. And it seems like most of the tests and documentation just render the slot, which sidesteps the issue, because at the point of rendering the slot's template, you're inside the component, not the slot, and so you can call whatever methods you have defined without interference from the Slot class hierarchy.

Even still, given what SlotV2 is trying to accomplish (it's essentially a specialized delegator object) I think that BasicObject is the better parent class for it to have. But testing it would require using it in a way that is currently not documented, and not tested in any other tests. I'm happy to take a stab at writing that test, but I wanted to bring these thoughts to the developers of this library before I started marching off the beaten trail.

@joelhawksley just bumping this to see if this is still something you're interested in merging, or if I should go ahead and close it.

The TL;DR recap is: I use slots a bit differently than is perhaps intended, and because SlotV2 inherits from Object instead of BasicObject, there is a larger surface area of method names that I am not permitted to use on my slotted objects. The specific one I was running into was with a Struct used in a slot that had a field called method to represent the HTTP method that the link should make, but method is defined on SlotV2 because it currently inherits from Object, so I am unable to access my method field on the struct because SlotV2 will not delegate if it has it's own definition.

So I proposed a change to make SlotV2 inherit from BasicObject, similar to how the Ruby stdlib Delegator class works.

I got stuck when you asked for a test necessitating that change because it either requires testing using my unconventional ViewComponent usage of using Structs instead of other ViewComponents in slots (shown in detail above), or having a very contrived test like refute_operator SlotV2, :<, Object

👋 @willcosgrove thanks again for opening this! Looking at the example above, I'd say that's not how we originally envisioned slots being used, and while it works for now, it's possible that future updates to the slots feature may break that behavior and not be considered a breaking change. However, I do think this PR is a great addition and would love to merge the feature as long as both this repo and the primer view component repo's tests pass.

Re: test cases, I think a good test case would be finding a method on Object and ensuring that it gets called on the component instance instead of the slot instance.

@BlakeWilliams I've added a test, but I'm not very happy with it. I'm not super familiar with Minitest, and this seems like the ideal use case for a test spy, but Minitest doesn't provide something like that without pulling in an extra development dependency, which I didn't want to do.

Maybe someone with a better understanding of Minitest could take a stab at improving it. But as of right now, the test I added fails when SlotV2 does not specify a superclass, but passes when the superclass is set as BasicObject

It looks like the errors this is getting on PVC is from components that are using this "hack":

https://github.com/primer/view_components/blob/ff9813f1fe46ef279436a18a4a0107ff264667be/app/components/primer/popover_component.rb#L57-L59

https://github.com/primer/view_components/blob/ff9813f1fe46ef279436a18a4a0107ff264667be/app/components/primer/dropdown.rb#L9-L14

I haven't been able to work out exactly how that is working, or why it's necessary. Maybe @manuelpuyol could chime in as it looks like he had a hand in designing these APIs and may have some idea why changing SlotV2's superclass to BasicObject would be causing these components to blow up.

hey @willcosgrove that hack allows us to use lambda slots, which configures some attributes in the parent component, while rendering the passed content without having to create a new content-only component.
I do think this is more confusing that it needs to be so I opened a PR to stop using that hack in favor of a new component in primer/view_components#922

Closing as stale. Please reopen if you'd like to continue your work here ❤️