Better handling of splatted keyword arguments and with_collection

[daniel-rikowski]

Feature request

Allow components to have unnamed keyword arguments when using with_collection

Motivation

This would allow using generic initialize methods with unnamed parameters and provide more flexible opportunities for composition, especially in regards of DRYing up initializers.

Also, I can imagine, this would become a necessity with #2004

Details

Currently these scenarious are not possible:

Example 1: (concept from ViewComponentContrib)

class ApplicationViewComponent < ViewComponent::Base
  extend Dry::Initializer[undefined: false]
end

class ButtonComponent < ApplicationViewComponent
  option :button

  erb_template <<~ERB
    <%= button.inspect %>
  ERB
end

NB: extend Dry::Initializer adds this method definition:

def initialize(...)
  __dry_initializer_initialize__(...)
end

(... is verbatim Ruby code)

Example 2: (meta programming)

module InitHelpers
  extend ActiveSupport::Concern

  class_methods do
    def record_name = name.demodulize.underscore.chomp("_component").to_sym
  end

  included do
    attr_reader self.record_name
  end

  def init_component(**options)
    attr = self.class.record_name
    instance_variable_set(:"@#{attr}", options[attr])
  end
end

class AvatarComponent < ViewComponent::Base
  include InitHelpers

  def initialize(**)
    init_component(**)
  end

  erb_template <<~ERB
    <%= avatar.inspect %>
  ERB
end

There is an explicit check to exclude unnamed keyword arguments:

view_component/lib/view_component/base.rb

Lines 695 to 698 in 66dcb7c

	def splatted_keyword_argument_present?
	initialize_parameters.flatten.include?(:keyrest) &&
	!initialize_parameters.include?([:keyrest, :**]) # Un-named splatted keyword args don't count!
	end

I did some rummaging and found this commit 56a6560 It mentions that Ruby 3.1 def m(*) might implicitly be the same as def m(*,**)

If that's actually the case it does make sense to forbid unnamed splatted kwargs, but I assume this check can be relaxed for other Ruby versions.

PR

I'm happy to make a PR, but first I want to discuss the proper way to do this, especially considering the fact, that removing the check might introduce subtle bugs.

[reeganviljoen]

@daniel-rikowski would you be able to add some failing tests that we could use as a spec

[daniel-rikowski]

@reeganviljoen How shall I approach this? Do you want me to create a PR with just the failing tests? Or shall I post them here before?

Anyway, I think I found the root cause.

If you have a component without an initialize method, instance_method(:initialize) in this line

view_component/lib/view_component/base.rb

Lines 708 to 710 in 66dcb7c

	def initialize_parameters
	@initialize_parameters ||= instance_method(:initialize).parameters
	end

returns ActionDispatch::Routing::UrlFor#initialize(...). 😮

This stems from the ActionView::Base parent class of all view components. (I have no idea why this doesn't return ViewComponent::Base#initialize or ActionView::Base#initialize.)

In other words: Without an overridden initialize method, the splatted_keyword_argument_present? method actually checks ActionDispatch::Routing::UrlFor#initialize. (So no Ruby bug 😌 😄)

The following change does seem to fix the whole problem (still, some tests need to be adapted)

      def initialize_parameters
        # Instead of:
        # @initialize_parameters ||= instance_method(:initialize).parameters
        # This:
        @initialize_parameters ||=
            instance_method(:initialize).then { |m| m.owner < ViewComponent::Base ? m.parameters : [] }
      end

[reeganviljoen]

@daniel-rikowski i have been quite busy lately but I will see if I can find some time to add failing tests and then your research should really come in handy

[daniel-rikowski]

I invested a few hours over the weekend. Although the failing tests and the fix are fairly simple, I can't figure out when or why instance_method(:initialize) returns ActionDispatch::Routing::UrlFor#initialize(...) instead of ViewComponent::Base#initialize(*).

Here's are failing tests:

# frozen_string_literal: true

require "test_helper"

module ViewComponent
  class UnnamedArgumentsTest < TestCase
    class DynamicComponentBase < ViewComponent::Base
      def setup_component(**attributes)
        # This method is somewhat contrived, it's intended to mimic features available in the dry-initializer gem.
        model_name = self.class.name.demodulize.delete_suffix('Component').underscore.to_sym
        instance_variable_set(:"@#{model_name}", attributes[model_name])
        define_singleton_method(model_name) { instance_variable_get(:"@#{model_name}") }
      end
    end

    class OrderComponent < DynamicComponentBase
      def initialize(**)
        setup_component(**)
      end
      def call
        "<div data-name='#{order.number}'><h1>#{order.number}</h1></div>".html_safe
      end
    end

    class CustomerComponent < DynamicComponentBase
      def initialize(...)
        setup_component(...)
      end
      def call
        "<div data-name='#{customer.name}'><h1>#{customer.name}</h1></div>".html_safe
      end
    end

    def setup
      @customers = [OpenStruct.new(name: "Taylor"), OpenStruct.new(name: "Rowan")]
      @orders = [OpenStruct.new(name: "O-2024-0004"), OpenStruct.new(name: "B-2024-0714")]
    end

    def test_supports_components_with_argument_forwarding
      render_inline(CustomerComponent.with_collection(@customers))
      assert_selector("*[data-name='#{@customers.first.name}']", text: @customers.first.name)
      assert_selector("*[data-name='#{@customers.last.name}']", text: @customers.last.name)
    end

    def test_supports_components_with_unnamed_splatted_arguments
      render_inline(OrderComponent.with_collection(@orders))
      assert_selector("*[data-name='#{@orders.first.number}']", text: @orders.first.number)
      assert_selector("*[data-name='#{@orders.last.number}']", text: @orders.last.number)
    end
  end
end

and this is the fix:

module ViewComponent
  class Base < ActionView::Base
    class << self
      def splatted_keyword_argument_present?
        # initialize_parameters.flatten.include?(:keyrest) &&
        #   !initialize_parameters.include?([:keyrest, :**]) # Un-named splatted keyword args don't count!
        initialize_parameters.flatten.include?(:keyrest)
      end
    end
  end
end

BUT:

Now RenderingTest#test_collection_component_missing_default_parameter_name fails.

That component has no own initialize method:

class MissingDefaultCollectionParameterComponent < ViewComponent::Base
  def call
  end
end

So it doesn't (=shouldn't) accept a collection argument and the test verifies, that ViewComponent detects this.

But in this case it fails, because instance_method(:initialize) returns ActionDispatch::Routing::UrlFor#initialize(...) which does accept it.

In other words:

instance_method(:initialize) returns #<UnboundMethod: ActionDispatch::Routing::UrlFor#initialize(...) > while
method(:initialize) returns #<Method: #<Class:MissingDefaultCollectionParameterComponent>(Class)#initialize(*)>

I have no idea why this happens, or how to circumvent it. My original idea to check instance_method(:initialize).owner < ViewComponent::Base would work, but that would also disable any initialize method coming from a module, e.g. Dry::Initializer.

I tried to reproduce this with minimal code, but I was unable to define a class in which instance_method(:initialize) and method(:initialize) pointed to different methods. I guess my Ruby knowledge is just limited here.

[MyklClason]

Thanks for referring to my abstract components issue. Reviewing what you have here, I'm not sure what you are suggesting is ideal. There is some merit in avoiding duplication via the DynamicComponentBase class, but there's actually a good reason why I suggest creating the base components. This is because the base component works really good as a place to put helper methods and other things that may be used across components that use the same base component. Concerns are a better solution for helpers not limited to a specific kind of input.

Edit: Near the bottom is a solution to DRY up the initializers though.

Indeed, rather than adding a dynamic base class. It is probably more helpful to modify the generators or make it easier to create your own custom generators that make use of dry-effects if you want.

Here is how I would implement the OrderComponent and CustomerComponent you had. I used two different ways of handling "data_key" and "data_value". The first moves it to the base class, and the other just handles the key and value inline.

class DesignSystem::H1Component < Record::BaseComponent
  attr_reader :key, :value
  
  def initializer(key:, value:)
    @key = key
    @value = value
  end
  
  def call
    "<div data-#{key}='#{value}'><h1>#{value}</h1></div>".html_safe
  end
end

class Order::BaseComponent < ApplicationComponent
  attr_reader :order
  
  def initialize(order:)
    @order = order
  end

  def data_key
    :number
  end
  
  def data_value
    order.number
  end
end

class Order::H1Component < Order::BaseComponent
  def call
    DesignSystem::H1Component.new(key: data_key, value: data_value)
  end
end

class Customer::BaseComponent < ApplicationComponent
  attr_reader :customer
  
  def initialize(customer:)
    @customer = customer
  end
end

class Customer::H1Component < Customer::BaseComponent
  def call
    DesignSystem::H1Component.new(key: :name, value: customer.name)
  end
end

However, to solve the original issue, I did have ChatGPT assist with quickly generating a solution that uses class_eval. Though I used it for the example above, so it will work with things like: Customer::TableComponent.new(customer:) and Customer::Invoice::TableComponent.new(customer_invoice:). It also works with Customer::Invoice::TableComponent.new() which is the same as Customer::Invoice::TableComponent.new(customer_invoice: nil)

class DynamicComponentBase < ViewComponent::Base
  def self.inherited(subclass)
    super
    subclass.class_eval do
      parts = name.split("::").map(&:underscore)
      parts = parts[0..-2] if parts.size > 1
      component_name = parts.join("_")
      
      attr_reader component_name

      define_method(:initialize) do |**kargs|
        instance_variable_set("@#{component_name}", kargs[component_name.to_sym])
      end
    end
  end
end

I use it myself to dry up the places where an initializer isn't needed.

Edit: Here it is as a concern.

module BaseComponentInitializer
  extend ActiveSupport::Concern

  included do
    component_name = extract_component_name(name)

    class_eval do
      attr_reader component_name

      define_method(:initialize) do |**kargs|
        instance_variable_set("@#{component_name}", kargs[component_name.to_sym])
      end
    end
  end

  class_methods do
    private

    def extract_component_name(class_name)
      parts = class_name.split("::").map(&:underscore)
      parts = parts[0..-2] if parts.size > 1
      parts.join("_")
    end
  end
end

class Order::BaseComponent < ApplicationComponent
  include BaseComponentInitializer

  def data_key
    :number
  end
  
  def data_value
    order.number
  end
end

def Customer::BaseComponent < ApplicationComponent
  include BaseComponentInitializer
end

Edit 2: It seems there are too many possible ways to auto-extract component names when it comes to namespacing things IE: "Record::Buttons::NewComponent" vs "Customer::TableComponent" vs "Customer::Invoice::TableComponent" to the point where I figured I might as well just explicitly define the model name. As such, I have created this instead:

module BaseComponentInitializer
  extend ActiveSupport::Concern

  included do
    def self.base_initializer(model_name:)
      class_eval do
        attr_reader model_name

        define_method(:initialize) do |**kargs|
          instance_variable_set("@#{model_name}", kargs[model_name.to_sym])
        end
      end
    end
  end
end

This allows you to just use this:

    class OrderComponent < ApplicationComponent
      include BaseComponentInitializer
      base_initializer(model_name: :order)
      
      def call
        "<div data-name='#{order.number}'><h1>#{order.number}</h1></div>".html_safe
      end
    end

    class CustomerComponent < ApplicationComponent
      include BaseComponentInitializer
      base_initializer(model_name: :customer)
      
      def call
        "<div data-name='#{customer.name}'><h1>#{customer.name}</h1></div>".html_safe
      end
    end

include BaseComponentInitializer could be moved to a superclass if suitable.

Edit 3: Allow dynamic attributes.

module Abstract::Concerns::BaseComponentInitializer
  extend ActiveSupport::Concern

  included do
    def self.base_initializer(*attributes)
      class_eval do
        attr_reader *attributes

        define_method(:initialize) do |**kargs|
          attributes.each do |attribute|
            instance_variable_set("@#{attribute}", kargs[attribute.to_sym])
          end
        end
      end
    end
  end
end


class CustomerComponent < ApplicationComponent
  include BaseComponentInitializer
  base_initializer :customer
  
  def call
    "<div data-name='#{customer.name}'><h1>#{customer.name}</h1></div>".html_safe
  end
end

[daniel-rikowski]

@MyklClason Now I feel bad, because you put so much work into your answer 😩 These are all good ideas, but I'm not looking for new ways to implement these features.

The code from the test was actually just intended to mimic some of the functionality of dry-initializer without introducing that gem as a new dependency into the test suite.

In a nutshell, I want to have this, more or less: https://github.com/palkan/view_component-contrib#hanging-initialize-out-to-dry

I.e. be able to do this...

class ApplicationViewComponent
  extend Dry::Initializer
end

class CustomerComponent < ApplicationViewComponent
  option :customer
end

... and be done with it.

But ViewComponent actively prevents these components to be used in a collection.

(They need to have an explicit initialize method with named arguments, while a major purpose of dry-initializer is getting rid of said methods.)

[daniel-rikowski]

Anyway, even if it is decided, that my use case is not worth the trouble, it is still a possibility, that some day the Rails developers change the signature of the method, which made this whole workaround necessary.

E.g. they change ActionView::Routing:UrlFor#initialize from this

def initialize(...)
  @_routes = nil
  super
end

to - let's say - this:

def initialize(*, **options)
  @_routes = nil
  @_options = options
  super
end

(Unlikely, admittedly, but it is a possibility.)

Then, the afformentioned parameters check would no longer help in preventing misconfigured components, because then, many components get their named splatted arguments from ActionView::Routing:UrlFor#initialize (Without their developers ever knowing...)

IMHO this is a much deeper problem than just preventing certain component class definitions.

[MyklClason]

@daniel-rikowski

Lol, no problem. My initializers are more DRY now lol. So I'm happy enough.

However, isn't isn't that what the last edit did?

class ApplicationViewComponent
  include BaseComponentInitializer # extend Dry::Initializer
end

class CustomerComponent < ApplicationViewComponent
  base_initializer :customer # option :customer
end

Of course, you can change the method name so it uses "option" instead of "base_initializer".

I have seen the "_routes" you mention, but I just ignore it.

Here is a version that in theory mimics what you had except it uses "include" rather than "extend". It's been a while since I looked into the differences between those.

module Dry::Initializer
  extend ActiveSupport::Concern

  included do
    def self.option(*attributes)
      class_eval do
        attr_reader *attributes

        define_method(:initialize) do |**kargs|
          attributes.each do |attribute|
            instance_variable_set("@#{attribute}", kargs[attribute.to_sym])
          end
        end
      end
    end
  end
end

class ApplicationViewComponent
  include Dry::Initializer
end

class CustomerComponent < ApplicationViewComponent
  option :customer
end

Edit: Probable solution below

Oh I think I see what we are missing. We also need to make use of "with_collection_parameter" - https://viewcomponent.org/guide/collections.html#with_collection_parameter

In that case, we need to make two adjustments:

1. We cannot support multiple attributes. It can only be a single attribute. Edit 2: This actually isn't the case, but it simplifies the work. See Edit 2 below.
2. we make use of "with_collection_paramter" as the attribute.

So we end up with this instead:

module Dry::Initializer
  extend ActiveSupport::Concern

  included do
    def self.option(attribute)
      class_eval do
        attr_reader attribute
        with_collection_parameter attribute

        define_method(:initialize) do |**kargs|
          instance_variable_set("@#{attribute}", kargs[attribute.to_sym])
        end
      end
    end
  end
end

class ApplicationViewComponent
  include Dry::Initializer
end

class CustomerComponent < ApplicationViewComponent
  option :customer
end

And here is the working version of my own (in the off chance I didn't change everything in the above version)

module Abstract::Concerns::BaseComponentInitializer
  extend ActiveSupport::Concern

  included do
    def self.base_initializer(attribute)
      class_eval do
        attr_reader attribute
        with_collection_parameter attribute

        define_method(:initialize) do |**kargs|
          instance_variable_set("@#{attribute}", kargs[attribute.to_sym])
        end
      end
    end
  end
end

Edit 2: Looks like some additional logic could allow this to support multiple attributes based on this: https://viewcomponent.org/guide/collections.html#additional-arguments