Conversation
joelhawksley
changed the title
|
3 file(s) had their final line ending fixed:
|
|
1 file(s) had their final line ending fixed:
|
|
@joelhawksley does this now mean that a component could have a multiple format templates, i.e the same component has a json and an html template? |
BlakeWilliams
left a comment
BlakeWilliams
left a comment
There was a problem hiding this comment.
I left a few comments, but I think my comment about considering a separate class for handling some of the template logic is the one I'm curious about most. I think that could help reduce the complexity of the compiler a good bit, and would be happy to pair or chat more about it!
lib/view_component/base.rb
Outdated
| end | ||
|
|
||
| def safe_render_template_for(variant) | ||
| def safe_render_template_for(variant, format = nil) |
There was a problem hiding this comment.
| def safe_render_template_for(variant, format = nil) | |
| def safe_render_template_for(variant, format: nil) |
extremely minor, but thoughts on using a kwarg for nice readability?
There was a problem hiding this comment.
My Personal Ruby Style Guide ™️ says to only use kwargs for > 2 arguments 🤷🏻
Regardless, this method is only used once, so I'm just going to inline it. No point in having default values for a single callsite anyways.
| def define_render_template_for | ||
| variant_elsifs = variants.compact.uniq.map do |variant| | ||
| safe_name = "_call_variant_#{normalized_variant_name(variant)}_#{safe_class_name}" | ||
| branches = [] |
There was a problem hiding this comment.
This is getting pretty complex, is this something we could break into more methods, or perhaps a separate class that's responsible for returning template objects that define the relevant methods? Like name, method_name, format, etc.?
It might also make the testing story a bit easier and more comprehensive since we don't have to couple the compiler and template logic as heavily.
There was a problem hiding this comment.
Oh man, all I wanted to do while writing this PR was refactor this entire file, but I held back. I'm happy to go down that route, but maybe it would be best to do as a follow-up?
There was a problem hiding this comment.
Hah, the compiler has gotten a bit gnarly! Part of the reasoning was that I had a hard to following the code in here (but I also didn't have a ton of time to dedicate to reading it) so I thought refactoring might make it easier to follow the changes.
One strategy I've done in the past for changes like this is refactoring the PR on-top of the new behavior, backporting it while removing the new functionality as a separate PR (so 1-to-1 functionality wise), then re-adding those initial refactor changes on-top. It's a bit roundabout, but I found it helps reduce churn and makes the changes easy-to-follow.
I'm also fine with a follow-up, but I'd also say I haven't reviewed this code in depth yet if we wanted to go that route. 🙂
lib/view_component/compiler.rb
Outdated
| end | ||
|
|
||
| if templates.count { |template| template[:variant].nil? } > 1 | ||
| if templates.map { |template| "#{template[:variant].inspect}_#{template[:format]}" }.tally.any? { |_, count| count > 1 } |
There was a problem hiding this comment.
Why do we use inspect here?
| ) | ||
| end | ||
|
|
||
| def test_renders_multiple_format_component_as_html |
There was a problem hiding this comment.
Similar to Rails default behavior do we want a test that validates we fall back to HTML in cases where the component is missing a format specific template?
Should we also have a test that validates that ViewComponent respects the formats option passed to render if we don't have one already?
There was a problem hiding this comment.
Hmm. My read of the docs is the opposite, that we should _not _ fall back for an unknown format:
If a template with the specified format does not exist an ActionView::MissingTemplate error is raised.
There was a problem hiding this comment.
Also, I wrote a test for the formats option and realized that we don't pass any of the render arguments into components: https://github.com/rails/rails/blob/4bb2227640531c877e30cc96f10df0c298dc3331/actionview/lib/action_view/template/renderable.rb#L16.
IMO we should look at passing render options to renderables, as it would be nice to handle format overrides.
There was a problem hiding this comment.
Ohhhh, you're right. I read that wrong. Good catch.
I was reading:
Rails uses the format specified in the request (or :html by default)
Which I think means instead of passing format = nil in the compiler, it should be format = :html? 🤔
There was a problem hiding this comment.
It looks like we're passing :html by default in the compiler: https://github.com/ViewComponent/view_component/pull/2079/files#diff-fa28fa6e2d5f267384e7793b855281451a46b8437431867871a298fc52fe4940R316
I believe request&.format&.to_sym will also be :html by default, which gets passed to render_template_for.
Co-authored-by: Blake Williams <blakewilliams@github.com>
camertron
left a comment
camertron
left a comment
There was a problem hiding this comment.
This all makes sense to me 😄
4 participants
What are you trying to accomplish?
I'm looking to more holistically address #1990 by having ViewComponent properly handle formats instead of hardcoding to HTML.
What approach did you choose and why?
I passed the format from
render_indown to the template selection logic, which now allows for multiple formats in a single component ❤️I've also added a few test helpers to make working with formats more enjoyable.
Anything you want to highlight for special attention from reviewers?
Nothing in particular. There is no way I could have pulled this off without our massively thorough test suite ❤️
cc @fredboyle
Closes #1990 #1783