feat: Limit templates to defined file extension

[fredboyle]

What are you trying to accomplish?

• Limit ViewComponent template to a specific file extension
  • Defaults to ‘html’ unless overriden by config setting template_extension
• Allows use of .erb processed files such as CSS or JavaScript to be in sidecar location with HTML template(s)
• Adds the template_extension config setting to specify a custom file extension to use as the template
• Addresses concerns discussed in Error when using .css.erb sidecar file for component #1783

Anything you want to highlight for special attention from reviewers?

Unclear how to properly run tests for this repo. Clarification from someone with more intimate knowledge of this repo would be appreciated.

[fredboyle]

@boardfish Checking in to see if I could get assistance in completing this and getting the code reviewed and eventually merged in. Thanks.

[joelhawksley]

@fredboyle thanks for your PR. Might you provide docs and/or tests that necessitate this change?

[fredboyle]

@fredboyle thanks for your PR. Might you provide docs and/or tests that necessitate this change?

@joelhawksley Gladly. Would you kindly point me to the best location for tests? It will be config variation testing and invalid path tests. However it's unclear to me where these tests live as well as how to run the tests in this repo.

Thanks for your guidance and help.

[joelhawksley]

@fredboyle I'd look at existing tests and see what you think is the closest fit.

Perhaps we might be better off setting this at the component level though? I could see setting it on ApplicationComponent.

[fredboyle]

@fredboyle I'd look at existing tests and see what you think is the closest fit.

Perhaps we might be better off setting this at the component level though? I could see setting it on ApplicationComponent.

Ah a good point regarding the component level configuration instead of system wide. Was not aware that was part of the architecture. I'll investigate and update the pull-request.

[fredboyle]

@joelhawksley Updated things to be more flexible with sensible defaults and added a test. Please take a look and let me know if this fits into the ViewComponent approaches. Thanks.

[joelhawksley]

@fredboyle thanks for writing this PR!

After giving it a read, I'm wondering if what we really need to do is allow folks to explicitly set the format for a component. If we did, you could have multiple ERB files and VC would just use the one for the selected format, which is currently HTML by default. What do you think about going that direction?

[fredboyle]

@joelhawksley This is what this pull-request permits a user to do.

By default ViewComponent scans the files for each handler, the default handler being ERB if it finds more than one file it reports too many templates were found. It doesn't distinguish between file types within a handler. Unless I missed something, possible since I'm not super familiar with this codebase.

ViewComponent defaults to rendering HTML as the template but only if no errors were triggered by the handler file search.

The code in this pull-request allows a user to have as many files as needed per handler without triggering the error AND also set which file type will be used to render the component. So there could be .css.erb, .html.erb, .js.erb files for the component but if the component has VC_TEMPLATE_EXTENSION="html" it would render the HTML template file processed by ERB without a too many templates found error.

Hoping my understanding is correct and my explanation makes sense. Glad to hop on a live call if helpful to sync up and discuss.

[joelhawksley]

@joelhawksley This is what this pull-request permits a user to do.

By default ViewComponent scans the files for each handler, the default handler being ERB if it finds more than one file it reports too many templates were found. It doesn't distinguish between file types within a handler. Unless I missed something, possible since I'm not super familiar with this codebase.

ViewComponent defaults to rendering HTML as the template but only if no errors were triggered by the handler file search.

The code in this pull-request allows a user to have as many files as needed per handler without triggering the error AND also set which file type will be used to render the component. So there could be .css.erb, .html.erb, .js.erb files for the component but if the component has VC_TEMPLATE_EXTENSION="html" it would render the HTML template file processed by ERB without a too many templates found error.

Hoping my understanding is correct and my explanation makes sense. Glad to hop on a live call if helpful to sync up and discuss.

@fredboyle thank you for your explanation!

Giving this some more thought, I wonder if we should change the underlying behavior. I'd rather see us warn if there are multiple templates per format and/or variant and not worry about multiple templates per extension, assuming only one is HTML. What do you think about that approach?

[fredboyle]

@fredboyle thank you for your explanation!

Giving this some more thought, I wonder if we should change the underlying behavior. I'd rather see us warn if there are multiple templates per format and/or variant and not worry about multiple templates per extension, assuming only one is HTML. What do you think about that approach?

@joelhawksley Hmm not certain I'm clear on what you mean here. When you say format do you mean a handler such as ERB or HAML? If so then that's the problem I'm attempting to solve for. Multiple per handler causes issues if, for example, a component has both the HTML template to be rendered AND say the CSS sidecar file being processed by ERB.

I do tend to agree that HTML is the template file type for nearly all components. It may deviate from that standard but would be a rare exception. In my experience it would mostly be the handler/processor that changes and not the HTML nature of the template. So we could lock that down to only scan for HTML files regardless of handler.

If we take that approach then we no longer need to have a component configuration and we simply reduce the scope of what ViewComponent considers to be a template. In this case a .html.* file. The remaining logic would remain as is to handle any multiple variant scenarios. All other sidecar files would then be ignored and avoid any issues when multiple file types/extensions use the same handler as the template to be rendered.

Does that make sense? Are we saying the same thing?

Thanks again for your help in working through this.

[BlakeWilliams]

Is there a reason we wouldn't want to change the existing logic to look for multiple extensions and unique by that?

[fredboyle]

@BlakeWilliams I'm not clear on why a component would need multiple template file extensions. Could you provide an example to help me understand?

My understanding and experience is that a component renders a single type of output, usually HTML. So the variant rules apply to that HTML file type/extension if varying outputs are needed.

[BlakeWilliams]

I don't see why a component would care if there's both an HTML and a CSS file associated with it. The only case I think we care about is having two of the same filetype, like foo.html.erb and foo.html.haml, for example.

We also already have a way to determine the type of content a component should be rendering, via format

view_component/lib/view_component/base.rb

Lines 593 to 595 in 5ae3c12

	def format
	:html
	end

(at the class level)

[fredboyle]

@BlakeWilliams We're on the same page. The component should only care about what you mentioned. However the current functionality starts with the handler/processor and not the filetype such as .html.*.

Hence why there's a need to add a bit of code to limit the scope. This is to avoid the errors that arise if a component has more than one filetype per handler/processor.

Regarding the format at the class level you pointed out I did see that but I'm not certain it does much currently. However it doesn't mean that it shouldn't be leveraged in the solution we're aiming for. So tweaking things to use it is likely a good idea.

[fredboyle]

Checking in to see if there are any further thoughts on the discussions.
Apologies I haven't responded/checked-in sooner.

[reeganviljoen]

@BlakeWilliams do you have any further feed back for @fredboyle so we can maybe get some progress on this