Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add slot unwrapping API #1895

Closed
wants to merge 6 commits into from
Closed

Add slot unwrapping API #1895

wants to merge 6 commits into from

Conversation

camertron
Copy link
Contributor

@camertron camertron commented Nov 3, 2023 •
edited

Fixes #1737

What are you trying to accomplish?

This PR introduces an API for retrieving the underlying component instance inside ViewComponent::Slots. At the moment, getting a reference to the underlying component necessitates using instance_variable_get, eg:

class MyComponent < ViewComponent::Base
  renders_one :header, HeaderComponent
  renders_many :items, ItemComponent
end

render(MyComponent.new) do |component|
  # returns an instance of ViewComponent::Slot
  component.header

  # returns an instance of HeaderComponent
  component.header.instance_variable_get(:@__vc_component_instance)
end

This is awkward. The @__vc_component_instance is private and not designed to be accessed outside the Slot class.

What approach did you choose and why?

I propose the following API:

render(MyComponent.new) do |component|
  # returns an instance of HeaderComponent
  component.header_instance

  # returns an array of ItemComponent instances
  component.item_instances
end

This works for any component slot. For lambda slots, the *_instance methods are not defined.

Anything you want to highlight for special attention from reviewers?

Is the _instance suffix the best naming? Maybe _component would be better? Open to suggestions 馃槃

@camertron camertron mentioned this pull request Nov 3, 2023
Open
@camertron camertron marked this pull request as ready for review November 8, 2023 18:42
@camertron camertron changed the title WIP: unwrap slots Add slot unwrapping API Nov 8, 2023
@Spone
Copy link
Collaborator

Spone commented Nov 8, 2023 •
edited

Re: the suffix I think _component is clearer than _instance (... of what?)

Anyway I think both are more explicit than the initial unwrap idea.

EDIT: I haven't tried, but I'm curious about what happens when you call the method on a lambda slot?

@BlakeWilliams
Copy link
Contributor

Just writing this down real quick since I don't have the time to write up a longer response, but I'm still on the fence here. I get the intention behind this change, but I also think we should try to avoid having another API for accessing slots in addition to the APIs we already have.

@Spone
Copy link
Collaborator

Spone commented Nov 8, 2023 •
edited

Yeah my comment was only about the naming.

I've never had a use case for this (that could not be solved in another way) in the projects I'm working on, so I'm not especially for this addition.

I reckon it adds another layer of complexity to the Slots which are already the most complex part of ViewComponent (in terms of usage I mean).

@camertron
Copy link
Contributor Author

Thanks for your feedback @Spone and @BlakeWilliams! I agree it adds a bit more complexity to an already complex feature, and I also agree there are limited occasions for use. I have personally run into a similar issue to the one the OP describes in #1737 (comparing two wrapped component instances). I've also had occasion to unwrap slots when attempting to forward them to other components, and while debugging. In other words, I think the framework needs some way of unwrapping slots. If this isn't it, that's fine, but I would like to figure out the right approach.

@reeganviljoen
Copy link
Collaborator

I would like to agree with @camertron, I have also needed access to the base component in a slot via unwrapping when I was attempting to add #1620 , this may even unblock it and give us the chance at re-attempting to add it, however due to the uncertainty may I suggest we add this as an optional experimental feature similar to how inline templates where added until we can see if it really is useful in the real world

@camertron
Copy link
Contributor Author

We talked about this internally and have decided removing the Slot wrapper is the better way forward 馃憤

@camertron camertron closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BlakeWilliams BlakeWilliams Awaiting requested review from BlakeWilliams BlakeWilliams is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A method to unwrap a ViewComponent::Base subclass instance that is within a ViewComponent::Slot
4 participants
@camertron @Spone @BlakeWilliams @reeganviljoen