Skip to content
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

Merged
merged 8 commits into from
Nov 7, 2024

Conversation

sfnelson
Copy link

@sfnelson sfnelson commented Nov 4, 2024

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.

Copy link
Member

@joelhawksley joelhawksley left a 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 ❤️

lib/view_component/compiler.rb Outdated Show resolved Hide resolved
lib/view_component/compiler.rb Outdated Show resolved Hide resolved
@sfnelson
Copy link
Author

sfnelson commented Nov 4, 2024

@joelhawksley thanks, I've rebased and removed the unnecessary change to format in Compiler. I've left the change in Template because it was already exposed as format and it's consistent with the change I've made to caching on my WIP branch.

@sfnelson sfnelson force-pushed the template-details branch 2 times, most recently from 1f2aa99 to d948e40 Compare November 4, 2024 22:52
@sfnelson
Copy link
Author

sfnelson commented Nov 6, 2024

@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.

v4.benchmark.txt
template-details.benchmark.txt

Copy link
Member

@joelhawksley joelhawksley left a 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 ❤️

docs/CHANGELOG.md Outdated Show resolved Hide resolved
docs/CHANGELOG.md Outdated Show resolved Hide resolved
@sfnelson
Copy link
Author

sfnelson commented Nov 7, 2024

Note, tests are failing due to Ruby 3.3.6 release

@joelhawksley joelhawksley merged commit 86b0149 into ViewComponent:v4 Nov 7, 2024
13 of 15 checks passed
@sfnelson sfnelson deleted the template-details branch November 8, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants