-
Notifications
You must be signed in to change notification settings - Fork 429
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.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add slot unwrapping API #1895
Add slot unwrapping API #1895
Conversation
Re: the suffix I think Anyway I think both are more explicit than the initial EDIT: I haven't tried, but I'm curious about what happens when you call the method on a lambda slot? |
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. |
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). |
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. |
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 |
We talked about this internally and have decided removing the |
Fixes #1737
What are you trying to accomplish?
This PR introduces an API for retrieving the underlying component instance inside
ViewComponent::Slot
s. At the moment, getting a reference to the underlying component necessitates usinginstance_variable_get
, eg:This is awkward. The
@__vc_component_instance
is private and not designed to be accessed outside theSlot
class.What approach did you choose and why?
I propose the following API:
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 😄