-
Notifications
You must be signed in to change notification settings - Fork 444
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
Abstract View Components #2004
Comments
@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. |
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
While not likely needed, one could subclass 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 👍 |
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 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:
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 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.
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.
The text was updated successfully, but these errors were encountered: