gruntwork-io / boilerplate

A tool for generating files and folders ("boilerplate") from a set of templates
https://www.gruntwork.io
Mozilla Public License 2.0
158 stars 12 forks source link

Add partials feature #66

Closed bwhaley closed 3 years ago

bwhaley commented 3 years ago

This introduces a new layouts feature to boilerplate. For background, this comes from an idea @brikis98 proposed on the aws-architecture-catalog. With this feature, a Boilerplate template can use the define and template features of Go's text/template package to define reusable templates that can wrap other content. For full details, here's a direct link to the updated README.md.

Note that in the discussion linked above, the hope was to use a boilerplate function to dynamically look up a template. Unfortunately, unless I'm missing something (which is a definite possibility!), that won't work, at least not in any easy/sane way that I can see. Template functions are run at template execution time (e.g. when Execute() is called), meaning all the templates in the tree must already be parsed. We can't dynamically register another template in mid-Execute. Instead, we have to provide a map of layouts in the boilerplate.yml configuration, and pass those along to be Parsed and added to the template parse tree before running Execute.

I've also added a template helper function called isTemplateDefined to allow checking if a template is defined calling it. Without this, an undefined template throws an error, and we'd need multiple templates with a bunch of overlapping code.

bwhaley commented 3 years ago
  1. I think this feature isn't really about loading layouts, but templates. We're loading named Go templates into memory so you can render them throughout your boilerplate code. One way to use a template is as a layout, but it's not the only way.

Agreed. I only named it layouts because calling it templates risks overloading the term. The entire boilerplate project is about templates. Does having a configuration section in boilerplate.yml called templates with a list of templates to add just confuse everything? Perhaps the feature could be called _additionaltemplates instead?

  1. Instead of loading specific, individual files as layouts (i.e., templates), we should probably allow users to specify globs. E.g., ../templates/*.html to load all HTML files in the ../templates folder. That way, we can have a bunch of reusable templates there, and most of our boilerplate.yml files will need only a single extra line to pull them all in.

I considered this, and the package even exports a ParseGlob function to make this easy. The issue is that the templates are added using the base name of the file, and this could result in collisions. Note the comment from the docs (which I also mentioned in a code comment):

When parsing multiple files with the same name in different directories, the last one mentioned will be the one that results.

So we could specify a path with a glob, i.e. ../templates/*.html, and that should work fine since there can't be collisions in the same directory. But if we wanted to allow a list of template files to include, like:

- ../templates/*.html
- ../../template/*.html

then there is a potential for collisions. For that reason, the named approach seemed cleaner to me.

3.i Instead of using {{ template }}, we add a helper called dynamic_template and users can all our helper instead: {{ dynamic_template }}. This helper loads the file at path and then calls Go's rendering engine to render it with context . The advantage here is everything is 100% defined in Go templating, with no need for specifying the templates you need to load in boilerplate.yml. The drawback is that we have to maintain an extra helper instead of using the native {{ template }}.

I think I wasn't clear in the description of this PR. I do not believe this approach is viable. When using a Go template, there are two main steps:

  1. Parse (or ParseFiles, or ParseGlob) the template
  2. Execute (or ExecuteTemplate) the template

The template functions, like the dynamic_template function you propose, are called during Execute - not during Parse time. During Execute, any templates you want to use must already be in the parse tree. A dynamic_template could not add a new template to the tree dynamically, in mid-Execute.

That is, unless I'm mistaken. But from what I can tell, this isn't feasible. Happy to see counter evidence as I do think that's a cleaner solution.

3.ii We have boilerplate pre-parse all the files it's loading, automatically discover {{ template }} calls, automatically load the templates for all it finds, and then render everything. This allows us to use completely native Go templating features with no overhead for the user... But it also requires a bunch of extra parsing logic, which would have to be recursive (e.g., one template including another), so it's probably not worth the effort. That said, see this SO answer for something similar someone already built.

This would work, and I thought about it as well. However, as you observed, probably not worth the effort to add pre-parsing logic to boilerplate to discover any template calls.

For these reasons, I'm in favor of keeping the solutions as I've already designed it, perhaps with the change of renaming from layouts to _additionaltemplates ... or is there a better name than that?

brikis98 commented 3 years ago

Agreed. I only named it layouts because calling it templates risks overloading the term. The entire boilerplate project is about templates. Does having a configuration section in boilerplate.yml called templates with a list of templates to add just confuse everything? Perhaps the feature could be called _additionaltemplates instead?

Good point. I think a lot of templating languages call them partials.

I considered this, and the package even exports a ParseGlob function to make this easy. The issue is that the templates are added using the base name of the file, and this could result in collisions. Note the comment from the docs (which I also mentioned in a code comment):

When parsing multiple files with the same name in different directories, the last one mentioned will be the one that results.

So we could specify a path with a glob, i.e. ../templates/*.html, and that should work fine since there can't be collisions in the same directory. But if we wanted to allow a list of template files to include, like:

- ../templates/*.html
- ../../template/*.html

then there is a potential for collisions. For that reason, the named approach seemed cleaner to me.

Wait, why does the file name matter? I thought what mattered was the name used in the {{ define <NAME> }}? If there is a duplicate <NAME>, then sure, the last one overrides the previous one(s), but I think that's how it would work in Go templating anyway (e.g., if you had {{ define foo }} twice in the same file), wouldn't it?

I think I wasn't clear in the description of this PR. I do not believe this approach is viable. When using a Go template, there are two main steps:

  1. Parse (or ParseFiles, or ParseGlob) the template
  2. Execute (or ExecuteTemplate) the template

The template functions, like the dynamic_template function you propose, are called during Execute - not during Parse time. During Execute, any templates you want to use must already be in the parse tree. A dynamic_template could not add a new template to the tree dynamically, in mid-Execute.

It's not about adding a new template to the tree... The dynamic_template function would call Go's rendering engine from scratch—i.e., call Parse and Execute—and return a string. Although it seems like that's almost exactly what the include helper does already?

bwhaley commented 3 years ago

Wait, why does the file name matter? I thought what mattered was the name used in the {{ define }}? If there is a duplicate , then sure, the last one overrides the previous one(s), but I think that's how it would work in Go templating anyway (e.g., if you had {{ define foo }} twice in the same file), wouldn't it?

The name matters when using ParseFiles and ParseGlob because, according to the docs:

Since the templates created by ParseFiles are named by the base names of the argument files, t should usually have the name of one of the (base) names of the files. If it does not, depending on t's contents before calling ParseFiles, t.Execute may fail.

But I actually think we can make this work by using path.Base for the template. I have a prototype working in my local branch that accepts a boilerplate.yml like:

partials:
  - ../html/*.html
  - ../../morehtml/*.html

It's not about adding a new template to the tree... The dynamic_template function would call Go's rendering engine from scratch—i.e., call Parse and Execute—and return a string. Although it seems like that's almost exactly what the include helper does already?

I guess I don't quite follow how this would work. This behavior is indeed what include does. It actually evaluates a template and includes the output within the current template. But this doesn't help with partials/layouts. If any templates are defined with {{ define }} and/or invoked with {{ template }} in the included file, that won't be evaluated. That's why we need to actually specify the files (or globs, in this case) in the boilerplate.yml explicitly - so that they can be parsed and added to the template collection before the template is executed.

Rather than continue to go back and forth on this, I picked a time slot on your office hours tomorrow to try to knock it out. Hopefully we can chat then and get on the same page.

bwhaley commented 3 years ago

Thanks for the sync up this morning. I've updated the branch to rename the feature from layouts to partials, and also changed the interface to accept a list of globs. Added tests and updated docs. Will be using this branch during development of the aws-architecture-catalog for the moment. I may also merge in the includes branch here if it looks like that will be useful enough to keep around.

brikis98 commented 3 years ago

Thanks for the sync up this morning.

👍

Just to capture for posterity what we discussed: what I was suggesting with a dynamic_partials helper won't work, as there doesn't seem to be a way to pass the parse tree from a current rendering tree to a new one. So, for example, if we're rendering a template that contains {{ define foo }} and {{ define bar}} get to a {{ dynamic_partial my-partial . }}, we can certainly find and render my-partial dynamically, using a totally new render tree, but there's no obvious way to let it know that foo and bar were defined in the current render tree.

I've updated the branch to rename the feature from layouts to partials, and also changed the interface to accept a list of globs. Added tests and updated docs. Will be using this branch during development of the aws-architecture-catalog for the moment. I may also merge in the includes branch here if it looks like that will be useful enough to keep around.

Roger. Is this PR ready for another review pass then?

bwhaley commented 3 years ago

Thanks for capturing our conclusions. This is ready for a review at your convenience.

bwhaley commented 3 years ago

Thanks for the review, @yorinasub17!

bwhaley commented 3 years ago

Okay, I think this is about ready to ship!

bwhaley commented 3 years ago

Merging this one.