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

feat: Limit templates to defined file extension #1990

Closed

Conversation

fredboyle
Copy link

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.

- 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)

Signed-off-by: Fred Boyle <vortex+github@snark.one>
@fredboyle
Copy link
Author

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

@joelhawksley
Copy link
Member

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

@fredboyle
Copy link
Author

@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
Copy link
Member

@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
Copy link
Author

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

Add test and supporting content

Signed-off-by: Fred Boyle <vortex+github@snark.one>
Signed-off-by: Fred Boyle <vortex+github@snark.one>
Add user documentation
Update test example files

Signed-off-by: Fred Boyle <vortex+github@snark.one>
@fredboyle
Copy link
Author

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

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.

@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
Copy link
Author

fredboyle commented Mar 19, 2024

@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
Copy link
Member

@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
Copy link
Author

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


## Setting template file extension

Sometimes a component will warrant the use of multiple file types for a single template handler. An example would be ERB handled files for both HTML and CSS. By default ViewComponent will return a "More than one template found for" error. To avoid this set the `VC_TEMPLATE_EXTENSION` constant in your component to the file extension of the template you wish your component to use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def format
:html
end
(at the class level)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Author

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

@reeganviljoen
Copy link
Collaborator

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

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.

4 participants