-
Notifications
You must be signed in to change notification settings - Fork 450
Use ActionView::TemplateDetails for handling format and variant #2156
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
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.
Thanks for taking this on! I'll take a closer look once rebased ❤️
@joelhawksley thanks, I've rebased and removed the unnecessary change to |
@joelhawksley I think this code is ready for review. I'd think about this as a relatively minor internal refactor to ViewComponents that largely preserves compatibility at an API level – with the exception of dropping support for variants containing I've run the benchmarks on my local dev machine and the results for this change are well within the margin of error vs V4. Attached if you're interested. I have a second, larger branch ready to review but it's based on using Rails' TemplateDetails structure for describing format and variants, so I'd like to get this change merged first, if that's ok. I'll open a WIP PR that depends on this one. |
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.
I think this is great! Thank you so much for your contribution ❤️
Removes support for variant names containing `.`.
Note, tests are failing due to Ruby 3.3.6 release |
What are you trying to accomplish?
This change introduces
ActionView::TemplateDetails
as the primary abstraction for parsing and interpreting formats and variants. In a future PR I'll use this to implement template ordering based on the same logic that Rails uses for tie-breaks.Discussion in #2128
What approach did you choose and why?
Rails already has complex and well-used logic for parsing template file names. ViewComponent's differences do not seem intentional, and using the Rails logic should make it easier for ViewComponents to avoid inconsistencies in the future.
I've split up the logic for the three different template types into separate classes. This helps reduce the conditional logic by using polymorphism instead. I've taken small steps and avoided any structural changes, but this lays the groundwork for more significant changes to the compiler logic.
Anything you want to highlight for special attention from reviewers?
I have broken compatibility with the previously supported
variant.name
feature. Rails explicitly does not allow period characters in variant names.