-
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
Use ActionView::TemplateDetails for handling format and variant #2156
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 ❤️
707201d
to
3c7be76
Compare
@joelhawksley thanks, I've rebased and removed the unnecessary change to |
1f2aa99
to
d948e40
Compare
d948e40
to
75fac63
Compare
@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 `.`.
75fac63
to
9e30635
Compare
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.