-
Notifications
You must be signed in to change notification settings - Fork 440
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
Fix render_parent
when variants are involved
#1801
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I'm not excited about the extra complexity required to support variants.
IMO we should deprecate them since they don't fit well into component based architectures. Would you mind opening an issue for that as part of this change?
Re: deprecating variants, see: primer/view_components#2152 |
Co-authored-by: Blake Williams <blakewilliams@github.com>
…t/view_component into render_parent_with_variants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Co-authored-by: Blake Williams <blakewilliams@github.com>
…hods); add helpful #render_parent_to_string method
* Fix render_parent when variants are involved * Ok, yield seems to work * Got it working for variants and inline templates * Refactor a bit * Fix Vale grammar issues; add #render_parent test back in; fix linting issues * Merge functionality instead * wip * Fix tests * Clean up * Small fixes * Update docs/api.md Co-authored-by: Blake Williams <blakewilliams@github.com> * Fix linting issues * Regen docs * Update lib/view_component/compiler.rb Co-authored-by: Blake Williams <blakewilliams@github.com> --------- Co-authored-by: Blake Williams <blake@blakewilliams.me> Co-authored-by: Blake Williams <blakewilliams@github.com>
* Fix render_parent when variants are involved * Ok, yield seems to work * Got it working for variants and inline templates * Refactor a bit * Fix Vale grammar issues; add #render_parent test back in; fix linting issues * Merge functionality instead * wip * Fix tests * Clean up * Small fixes * Update docs/api.md Co-authored-by: Blake Williams <blakewilliams@github.com> * Fix linting issues * Regen docs * Update lib/view_component/compiler.rb Co-authored-by: Blake Williams <blakewilliams@github.com> --------- Co-authored-by: Blake Williams <blake@blakewilliams.me> Co-authored-by: Blake Williams <blakewilliams@github.com>
Fixes primer/view_components#2134
What are you trying to accomplish?
Improve the ability to render the parent's template, taking variants and deeper inheritance hierarchies into account.
Background
A few months ago we introduced a method called
#render_parent
that, as the name suggests, renders the parent component's template. It is useful in situations where one component inherits from another and it is desirable to render the parent's template within the child's. For example:Variants
Unfortunately
#render_parent
doesn't work very smoothly with variants. It assumes all ancestors support the same variants, i.e. respond to the same#call_<variant>
methods. In an app where all variants are known this may well be true and desirable, but not so in a component library which may not use variants at all, or may use different names for them (thinkphone
vsmobile
, for example).If the child component supports a variant but the parent does not, ViewComponent currently raises a
NoMethodError
. Along the same lines, if the child component doesn't support a variant but the parent does, the child will incorrectly render the non-variant template. In fact, one can imagine a 2x2 matrix of such scenarios between any two inherited components:As mentioned earlier, one of the components may live in an app and one may live in a library, making the two difficult to modify together.
Deep inheritance hierarchies (i.e. > 2)
Another shortcoming of
#render_parent
has to do with Ruby's object model. In Ruby,self
is always the type it was assigned at instantiation. The only way to move to a different level of inheritance is by callingsuper
, which executes the superclass's method. To every method in the hierarchy however, the type ofself
is still the original type, and not the super type. Let's look at an example.The method that does the actual work,
ParentComponent#what_am_i
, sees itself as an instance ofChildComponent
despite being defined in theParentComponent
class.Ruby's behavior in this regard is in contrast to a language like Java, in which objects appear to change their type at each level of the inheritance hierarchy. In Java, the type of
self
would beParentComponent
insideParentComponent#what_am_i
, even though we actually created an instance ofChildComponent
.Ok, so why is this important? Well, it means that the existing
#render_parent
method can only work with two levels of inheritance, i.e. a parent-child relationship. It breaks when confronted with a set of grandparent-parent-child relationships.Let's look at a simplified version of the
#render_parent
method:Using
method(:call)
is problematic. It will return the top-most method in the inheritance hierarchy which In our example isChildComponent#call
. For the reason mentioned above, this is true even if we call#method
inParentComponent
. Ruby does not provide a way to obtain a reference to the currently executing method, so there's no way to know where we are in the hierarchy, and therefore no way to know who the next parent is.Said another way,
#render_parent
finds the#call
method on the superclass - but which superclass? Sinceself
is always of typeChildComponent
,#render_parent
will always callParentComponent#call
. There is no way for it to callGrandparentComponent#call
because to do soself
would have to be of typeParentComponent
... which it can never be.What approach did you choose and why?
To address the problem, @BlakeWilliams and I modified the existing
#render_parent
method and made a few changes to the template compiler. Rather than define a#call
method, the compiler now defines a method that includes the name of the class it's defined in, eg.#call_parent_component
or#call_child_component
. Since these methods are unique, they can be called by any member of the inheritance hierarchy to render that specific component's template. This avoids the "which superclass?" problem described earlier. Since the compiler knows exactly which class it's compiling, it can easily find the superclass and thus deduce the method it should call to render the superclass's template.When inherited, the
#render_template_for
method defined on the child class is added to an array of unbound methods. As the inheritance hierarchy deepens, the array grows. The array ordering represents the order in which parent templates should be rendered, starting from the top-most child to the bottom-most parent. When a component is rendered, an instance variable counter is set to zero and incremented on each call to#render_parent
. This ivar is used to index into the unbound method array.Crucially, all this can be done without calling
super
. Special care must be taken to call the correct superclass method considering the current variant, whichsuper
cannot do. For example, if the child component does not support the variant but the parent does,#render_parent
will render the parent's variant template instead of the default one. In contrast,super
is only capable of calling the superclass method by the same name as the current one.Performance impact
My benchmarks have shown a negligible performance impact.
Co-authored with @BlakeWilliams