A method to unwrap a ViewComponent::Base subclass instance that is within a ViewComponent::Slot

[corasaurus-hex]

Feature request

I ran into an issue when trying to compare the equality of components wrapped in a ViewComponent::Slot.

Here's a simplified version of the problem:

class Cell < ViewComponent::Base
  def initialize(row:)
    @row = row
  end

  def first?
    @row.cells.first == self
  end

  def last?
    @row.cells.last == self
  end
end

class Row < ViewComponent::Base
  renders_many :cells, ->() {
    Cell.new(row: self)
  }
end

row = Row.new
row.with_cell{ "Content" }
row.with_cell{ "Other Content" }
row.with_cell{ "Even more content" }

a_cell = row.cells.first
a_cell.first? # => false

That ought to return true, I think, but @row.cells.first returns a ViewComponent::Slot wrapping the Cell instance and so the equality check fails.

So this feature request is rather open ended. There are a handful of ways I could see going with this:

1. Go way deeper on the proxy object route by inheriting from BasicObject and proxying equality methods to the underlying component.
2. Go a little deeper on the proxy object route by inheriting from BasicObject, add warnings to the BasicObject methods that won't be proxied to the underlying instance, and add a method to unwrap the underlying component.
3. Stick with the current level of proxying and add a method to unwrap the underlying component, maybe add some documentation about this.
4. Do nothing, maybe shrug a little, move on with our lives, go outside, have fun.
5. Probably other things but my imagination has its limits.

Thoughts? Comments? Jeers?

[boardfish]

@row.cells.first returns a ViewComponent::Slot wrapping the Cell instance

@__vc_component_instance on that object should contain the component instance to be rendered, in that case. I wonder if this means we should make this available on a public reader?

I think we'd have to give some consideration to the situations you might want to use this for – maybe we'd end up with situations where we do things differently at the parent level depending on which component was rendered in a polymorphic slot, for example. Opening up that sort of possibility might lead to increased coupling between components, but perhaps that's a useful sharp knife to have in the box.

[corasaurus-hex]

I like the reader idea. I think any solution to this is going to come down to either doubling down on proxying (meaning, being better about hiding that it's a proxy), offering an escape hatch like a reader, or rearchitecting so there aren't proxy objects.

The escape hatch is simplest. Getting proxy objects correct enough to nearly completely hide that it's a proxy is hard. Rearchitecting is costly and takes a lot of time, probably requiring escape hatches in the meantime.

So I guess I'm definitely on board with some kind of reader that returns the component instance.

[joelhawksley]

@corasaurus-hex @boardfish this is an interesting question! Perhaps we could look at adding first? and last? to the framework? I know others have asked for it in the past.

[corasaurus-hex]

@joelhawksley you could, for sure, but what if the person would like to know middle? or second from last? or third from last? what a headache 😩

It gets worse, though, with proxies. What if the person wants access to any of the other methods on the underlying instance that are not being proxied and won't be passed along by method_missing in Slot?

$ ruby -e 'puts Class.new.new.methods.sort_by(&:to_s).inspect'
[:!, :!=, :!~, :<=>, :==, :===, :__id__, :__send__, :class, :clone, :define_singleton_method, :display,
:dup, :enum_for, :eql?, :equal?, :extend, :freeze, :frozen?, :hash, :inspect, :instance_eval,
:instance_exec, :instance_of?, :instance_variable_defined?, :instance_variable_get,
:instance_variable_set, :instance_variables, :is_a?, :itself, :kind_of?, :method, :methods, :nil?,
:object_id, :private_methods, :protected_methods, :public_method, :public_methods,
:public_send, :remove_instance_variable, :respond_to?, :send, :singleton_class,
:singleton_method, :singleton_methods, :tap, :then, :to_enum, :to_s, :yield_self]

Oof, that's a lot...

Which is all to say, adding some variation of first? and last? might solve this one instance but it might leave the project on the hook for adding more and more of these sorts of methods in the future. It feels, to me, that it would simplest to make it more clear in docs that it's a proxy and to provide a way to retrieve the inner component from that proxy if you really need to. That way the user can do anything that want with it and VC doesn't need to maintain each use case. I mean, you could still add first? and last?, too, but I wouldn't skip adding some way of fetching the internal component.

In our case we've solved it by making our own base component:

class AppComponent < ViewComponent::Base
  def component_instance
    self
  end
end

class Cell < AppComponent
  def initialize(row:)
    @row = row
  end

  def first?
    @row.cells.first.component_instance == self
  end

  def last?
    @row.cells.last.component_instance == self
  end
end

class Row < AppComponent
  renders_many :cells, ->() {
    Cell.new(row: self)
  }
end

row = Row.new
row.with_cell{ "Content" }
row.with_cell{ "Other Content" }
row.with_cell{ "Even more content" }

a_cell = row.cells.first
a_cell.first? # => true

I imagine VC could do something similar?

[corasaurus-hex]

Oh! And I mentioned it before but if Slot inherited from BasicObject there would be more methods proxied via method_missing, which is likely something of an improvement. Since I showed all the methods that wouldn't be proxied above it seemed a good time to show the difference with BasicObject:

ruby -e 'puts Class.new(BasicObject).instance_methods.sort_by(&:to_s).inspect'
[:!, :!=, :==, :__id__, :__send__, :equal?, :instance_eval, :instance_exec]

It wouldn't help in this specific case, though, but I'm not sure if proxying == and similar is worth the headache of violating basic identity in ruby.

[Spone]

Perhaps we could look at adding first? and last? to the framework? I know others have asked for it in the past.

Yes, there's even an existing issue for it: #1620 🙂

[reeganviljoen]

@corasaurus-hex would this pr close this issue #1753

[corasaurus-hex]

@reeganviljoen it addresses the specific issue but not the general issue (specific: knowing the position of the component in the renders_many slot, general: proxy objects lead to surprising behaviors and benefit from proxying as much as possible and having a way to unwrap the internal object when you can't proxy a method). solving the former gets me past my immediate issue but solving the latter is more of a long-term approach that gives users a way to solve their own problems ¯\_(ツ)_/¯ -- so the answer is... maybe 😉

[reeganviljoen]

@corasaurus-hex I see your point, I would love to see the slot class refacrtored into the base class

[camertron]

Hey everyone, just coming back to this issue today and remembering a conversation a number of the maintainers had a few months ago. The problem with adding another method to Slot is that, because it's a proxy, any methods we define on it will implicitly cover up methods on the underlying component instance. I know that behavior has been very confusing in the past for me personally, and I have to imagine it's been confusing for others as well.

To @corasaurus-hex's point, a "perfect" proxy is probably not very feasible, and although we're considering removing the Slot proxy in ViewComponent v4, it would be nice to have a solution to this problem today.

Class method

One of the solutions we discussed was an unwrap class method, eg:

ViewComponent::Slots.unwrap(component_instance.some_slot)

Although an #unwrap instance method would be a lot nicer, using a class method avoids adding another method to the proxy.

Slot method

Another potential solution would be to add methods to the component class that would return the component instance instead of the slot proxy, eg:

component.some_slot_instance

These could be auto-generated by the existing Slotable code. We'd have to think about what to do for collection slots, i.e. those defined with renders_many.

Here's a proof-of-concept PR: #1895

[corasaurus-hex]

You could also go with a method name that is very unlikely to clash with the underlying object, like _view_component_slot_unwrap.

[camertron]

@corasaurus-hex good idea! That's a little verbose for my taste, but we could do vc_unwrap or something a bit shorter.

[corasaurus-hex]

Yeah, that would work, but the length was deliberate to further reduce the chance of collisions. Totally a subjective call on how likely collision might be, but I'd think including the full library name would help.

[Spone]

My concern with _view_component_slot_unwrap is that it reads like a private API. As a user, I would not rely on it.