Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for multiple formats #2079
Add support for multiple formats #2079
Changes from 14 commits
698e991
71f4adc
d356ce8
7c5cde5
10d68ee
229aaab
9560ca0
9640e67
4bacfd9
eee9aec
1331e41
ac048cb
1170071
7bbba51
c44b5b3
e46c231
955a52e
6fb636a
87978c8
b55a94a
61a6d61
e77aecd
9777f38
69a377f
0270cba
e0f057f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
extremely minor, but thoughts on using a kwarg for nice readability?
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂
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.
Why do we use
inspect
here?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.
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 torender
if we don't have one already?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.
Hmm. My read of the docs is the opposite, that we should _not _ fall back for an unknown format:
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhh, you're right. I read that wrong. Good catch.
I was reading:
Which I think means instead of passing
format = nil
in the compiler, it should beformat = :html
? 🤔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.
It looks like we're passing
:html
by default in the compiler: https://github.com/ViewComponent/view_component/pull/2079/files#diff-fa28fa6e2d5f267384e7793b855281451a46b8437431867871a298fc52fe4940R316I believe
request&.format&.to_sym
will also be:html
by default, which gets passed torender_template_for
.