Open MyklClason opened 2 months ago
@MyklClason I rather like this idea, it seems this is imported by how active record works which makes view_component acts more inline with rails which is what we aim for, @joelhawksley do you think we could make ViewComponent::Base one of these
@MyklClason interesting idea! I'm curious what practical uses we'd see come from this feature.
In ActiveRecord, abstract classes are used to allow for STI. I don't see an immediate need here beyond semantics, so I'm hesitant to add it. I'm guessing folks are already treating ApplicationComponent as an abstract class.
@joelhawksley
Interestingly enough, I'm not referring to it in how AR uses it, as that is fairly new. I'm more referring to it as a design pattern (so maybe it is mostly semantics, but it would also encourage better design patterns). For most use cases, if you need an abstract class, you can just use an implicit one or implement some logic to make one, as outlined in the link below. However, I think ViewComponent, like ActiveRecord, will benefit from being able to define an abstract class explicitly.
I agree that ApplicationComponent is a suitable abstract class. The issue is that, IMO, it is probably the only one most VC users will create, and only because of things like "ApplicationRecord" or they notice that it recommends it in the Getting Started Guide. I also highly doubt they will consider subclassing ViewComponents for a similar reason; only those that do a fair bit of subclassing anyway would consider it.
Rails has other places that use abstract classes.
https://api.rubyonrails.org/classes/AbstractController/Base.html
As for the implementation, I think the previous suggestion might not be ideal for you. For AR, an abstract class only needs to exist. I think the module above makes more sense. Here is a roughly example of one I use (with some potential abstract class logic):
Here is an example
class Record::BaseComponent< ViewComponent::Base
attr_reader :record
def initialize(record:)
@record = record
end
end
class DesignSystem::Record::LinkToButton < Record::BaseComponent
abstract_component missing_methods: %i(link_to_method) # Specifies abstract component because link_to_method is missing
# abstract_component :missing_template # Defines abstract component because it has no template
# abstract_component :missing_template, missing_methods: %i() would be valid as well.
# abstract_component :missing_template would error in this case as a template is actually provided in this class
# abstract_component logic maybe could checked immediate the class is loaded
erb_template <<-ERB
<%= link_to_method %>
ERB
def record_class
# This is used by Back/Cancel buttons
return record if record.is_a? Class
record.try(:klass) || record.class # klass is for associations
end
end
class Record::LinkToButton::DeleteButtonComponent < DesignSystem::Record::LinkToButton
def data
{turbo_method: :delete, turbo_confirm: "Are you sure?"}
end
def link_to_class
"btn btn-danger btn-sm"
end
def link_to_text
t(".destroy", default: t("helpers.links.destroy"))
end
def link_to_method
link_to link_to_text, record, class: link_to_class, data:
end
end
# Use Examples
<%= render Record::LinkTo::DeleteButtonComponent.new(record: user) %>
class Record::Tables::TableActionsComponent < Record::BaseComponent
erb_template <<-ERB
<%= turbo_frame_tag turbo_frame_id do %>
<%= render Record::LinkToButton::ShowButtonComponent.new(record:) %>
<%= render Record::LinkToButton::EditButtonComponent.new(record:) %>
<%= render Record::LinkToButton::DeleteButtonComponent.new(record:) %>
<% end %>
ERB
def turbo_frame_id
dom_id(record, :table_actions)
end
end
While not likely needed, one could subclass Record::LinkTo::DeleteButtonComponent
. More likely something like that would be needed for say a modal, where you would create a generic modal (that renders modal header/body/footer via slots) say DesignSystem::Modal::BaseComponent
, then subclass it to create say, DesignSystem::Modal::FormComponent
that wraps that in a form and maybe a turbo_frame that renders a generic modal say wrapped in a form with a turbo_frame around the form. Then you might create a User::Modal::FormComponent
that takes in a user and renders a form modal with the user inputs. Allowing for a straightforward solution to handling modals.
What this comes back to is Turbo Frames. We need a purpose for each component as it's easy to wrap a component in a turbo frame and then just render that component wherever you need that frame (maybe two layers of frames for when you need to replace a component with a similar one rather than exactly the same one, e.g., Replacing a User::DetailsComponent with an User::EditComponent"). Architecturally speaking, these are way easier to build if you plan for them in advance, and it's a lot easier to plan for something if you already have the concept in mind that "subclassing components can make things a lot less complicated" via a section specifically in the "How-to Guide" for it. Abstract classes seem like a good intro for that, and by explicitly defining an abstract component, it reinforces the concept as opposed to an implicit one.
What I'm suggesting now that I think about it is probably closer to attr_reader/attr_writer/attr_accessor in terms of use. One could manually define those as needed, but being able to use the DSL makes it cleaner and helps with best practices.
Ah, I think I have a better understanding now.
I think I'm coming around to this idea because it has always been awkward to subclass a component that already has a template and then define a new template. I think this approach would give us a better mental model around subclassing and should probably include a documentation guide with examples of when it is and isn't appropriate.
Would you be up for drafting a PR? I'd be interested in seeing the documentation fleshed out more than anything else. Once we agree on an API and feature set we can figure out implementation 👍
@joelhawksley
I'm happy you are following. I can certainly work on a PR draft and the documentation, but I'm about to go on vacation for a few days, so I probably won't be able to start on the PR until I get back. I could maybe do the documentation if I have some time.
Yeah, subclassing a VC with a template in order to define a new template is definitely an anti-pattern and should generally not be used precisely because it gets iffy and messy. As I gave examples above, it is valid to subclass either a) Add a missing method or b) Override a method. Though (b) is bad practice as it assumes that the class was already capable of rendering before it was overridden (which goes back to the original issue), hence the missing method approach. It seems best to leave the subclass to define the method and any helper methods needed to create it unless the helper methods are shared without change (such as the record_class
method, which could be used for back/cancel buttons).
My mental model is basically to categorize the kind of ViewComponents you can create by how they fit into the general architecture of the web app:
polaris_avatar
directly, but it is likely better to wrap it in a User::AvatarComponent
that takes in a user to create the avatar for. Which might become Profile::AvatarComponent
as you add profiles then become Avatar::StandardComponent
as part of an optimization of adding an Avatar Model/View Model/PORO. You might even want Avatar::NavComponent
to for the avatar in the navigation vs Avatar::ProfileComponet
for the avatar on the profile page. The last 2 are certainly Purpose-Built Components, but the other AvatarComponents might be more suitable as AbstractComponents for which a purpose can be defined.Record::BaseComponent
to be very effective as an abstract class. In the case where more than one input is needed, likely the solution is a View Model or View Object (PORO), which could be used to gather all the data that was required and then be passed along. For things like current_user,
the dry-effects gem can be used reasonably cleanly to pass counters and other "current state" type things that might need to be passed along that aren't directly related to the model (where it's not worth creating a View Component or View Object yet at least). Ideally, things like current_user are only considered in Purpose-Built Components, as only they are intended to interact with and be coupled to the app directly.I'm also slowly working on creating a course to cover this and more, but I'm happy to have it in VC directly. It's basically a "Rails Architecting with Hotwire and View Components" course.
@joelhawksley Sadly, I'm not feeling well, but I got a first draft done (mostly to transfer the work I did over the weekend in Trello to Github), it likely needs a lot of changes.
https://github.com/MyklClason/view_component/blob/abstract-classes/docs/guide/abstract.md https://github.com/MyklClason/view_component/blob/abstract-classes/docs/guide/best_practices.md
https://github.com/MyklClason/view_component/pull/1
It seems like Best Practices should be moved out of the "VC at GitHub" as it doesn't really allow for community recommendations for BP as any such recommendation would not be "How GitHub uses VC".
@joelhawksley So, it has been a bit busier than expected, and a bit lost where I was on this. Mind giving some feedback and comments on what the next steps should be in your opinion?
On a side note, along the way I did find this to be quite useful for cleaning up code, not really overly complicated, but rather useful when one just needs to render a single line of code anyway.
class DesignSystem::RubyRendererComponent < ViewComponent::Base
erb_template <<-ERB
<%= ruby_render %>
ERB
end
It allows for even less complicated view components. You just need to define a "ruby_render" method, and it will render whatever the result is.
Feature request
I have been using the concept of abstract view components (VC) quite a bit lately and I'm wondering if this can be formalized. The idea of an abstract VC is simply that it is a view component that is only intended to be subclassed and not directly called.
I'm thinking something as simple as one of these.
Running
AbstractComponent.new()
would error with something like "Cannot render abstract component: AbstractComponent". However, if you subclass it it will work fine.Motivation
The documentation doesn't really go into subclassing View Components, but I think that is one of the key benefits of essentially wrapping partials in classes.
Two main benefits in my mind for abstract components:
#1
above.Strictly speaking, you don't need to explicitly define the component as an abstract component, but I believe it would help with keeping things tidy. You might even just add logic that automatically defines anything as "::BaseComponent" as an abstract component by convention.