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

templates: Add template function "import" to enable {{ template ... }} and {{ block ...}} #4321

Merged
merged 8 commits into from
Sep 17, 2021

Conversation

rockorager
Copy link
Contributor

@rockorager rockorager commented Sep 1, 2021

Related to (closed) Issue #2094 on template inheritance. This PR adds a new function called "render" which works like "include", except it only takes one argument and passes it to the referenced file to be used as "." in that file.

Basic usage:

base.html

{{ $data := "This is data" }}
{{ render "/path/layout.html" $data}}

/path/layout.html

{{ . }}

Output

This is data

Possible use-case usage:

Use-case for me was to more cleanly pass markdown data to templates. Using include required having something like {{ $data := index .Args 0 }} at the beginning of any child template.

markdown.md

---
title: This is a title
---
This is body content

base.html

{{ $parsed := include "markdown.md" | splitFrontMatter}}
{{ render "/path/layout.html" $parsed}}

/path/layout.html

<h1>{{ .Meta.title }}</h1>
{{ markdown .Body }}

Output

<h1>This is a title</h1>
<p>This is body content</p>

Add {{ render "/path/to/file.ext" $data }} via funcRender
@CLAassistant
Copy link

CLAassistant commented Sep 1, 2021

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie changed the title caddyhttp/templates: Add template function "render" templates: Add template function "render" Sep 1, 2021
@francislavoie francislavoie added this to the v2.5.0 milestone Sep 1, 2021
@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Sep 1, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this seems like a handy change.

Do you think we can do it with less code repetition? Might require some slight refactoring.

@rockorager
Copy link
Contributor Author

Definitely. Here is what I did:

Call include from render with a unique Arg to trigger the Execute to send .Args[0]

func (c TemplateContext) funcRender(filename string, data interface{}) (returns) {
  render := "cmVuZGVy" // Unique string (render base64 encoded, actually)
  return c.funcInclude(filename, data, render)
}

And then modify executeTemplateInBuffer:

if len(c.Args) > 1 {
  if c.Args[1] == "cmVuZGVy" {
    return parsedTpl.Execute(buf, c.Args[0])
  }
}
return parsedTpl.Execute(buf,c)
}

Another option...
Same as above, but instead of sending a unique Arg, use import runtime to look in stack trace to see if calling function was funcRender. This one is more code, an extra import, but no risk of a user sending the unique arg into include (ie: {{ include $myArg "cmVuZGVy" }}

I already did the first way because I think it's better, but if you don't like it I can do this second way.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

That is definitely less repetition!

But the secret argument with a special value is not my favorite solution.

Can we instead refactor funcInclude slightly so that it has an "external" API (the one registered in the FuncMap for use in templates) and an "internal" API? Even if the only difference is a single parameter that specifies what value to use as the template context.

@rockorager
Copy link
Contributor Author

Hmm...I can't think of a way to do it by only refactoring funcInclude since that isn't telling executeTemplateInBuffer what to use for the context. I'm very green to this though so maybe there's an easy way I'm just totally missing?

But, I could add an argument to executeTemplateInBuffer to specify whether to use c or Arg[0]. This function gets called in two other places that would need to send that argument as well (HTTPInclude, and once in templates.go). Maybe something like

func (c TemplateContext) executeTemplateInBuffer(tplName string, buf *bytes.Buffer, mode int) error {
  ...
  // mode = 0, use c
  // mode = 1, use c.Arg[0]
  if mode == 1 {
    return parsedTpl.Execute(buf, c.Arg[0])
  }

  return parsedTpl.Execute(buf, c)

I'd have to change the two other calls to executeTemplateInBuffer to send a 0. And then I'd create an internal function that funcInclude and funcRender call which would in turn call executeTemplateInBuffer with the proper arguments, and no secret values.

Thoughts?

@mholt
Copy link
Member

mholt commented Sep 5, 2021

Thanks for working on this, it might be a while before I get around to this again since I have a conference coming up. Anyone else is welcome to give a review/feedback though in the meantime.

@rockorager
Copy link
Contributor Author

I rethought the problem and came up with a better solution.

  1. I added a field to the TemplateContext struct (tpl *template.Template)
  2. I refactored funcInclude to pull the file read portion out (used in funcImport now, too)
  3. I changed funcRender to funcImport. This function now takes in a filename, parses it (using a pointer to c.tpl so that it makes it into the parse tree). This does a few things:
    • It's really clean code now, doesn't require weird stuff with the Execute
    • It enables using {{ block ...}} and {{ template ...}}, see example below

Example of how it works:

import.html

{{ define "main" }}
This will be imported and rendered.
{{ end }}

index.html

{{ import "import.html" }}
{{ template "main" }}

You can also pass data into template, which will be used as . (like how I wanted render to work, but this is just free with the standard library).

You can also use {{ block ...}} from the standard library too.

Good luck with the conference, @mholt!

@rockorager rockorager changed the title templates: Add template function "render" templates: Add template function "import" to enable {{ template ... }} and {{ block ...}} Sep 9, 2021
@rockorager
Copy link
Contributor Author

Last commit is fixing a bug I found. I made funcImport return a nil error, but that was showing up in the html as <nil>. It now returns an empty string and the error, and nothing shows up in the html now.

@mholt
Copy link
Member

mholt commented Sep 14, 2021

Cool, looking forward to taking a look at this once I get caught up with some things!

@mholt
Copy link
Member

mholt commented Sep 16, 2021

@rockorager Do you know if something similar could be done to solve #2094? Do you think this is close enough?

@rockorager
Copy link
Contributor Author

This does solve the overwrite issue mentioned (as long as your imports are done above the block/template spots).

For example:

index.html

{{ import "post.md" }}
{{ block "content" .  }}
    This is my default content
{{ end }}

post.md

{{ define "content" }}
    This is post content
{{ end }}

Will render as

This is post content

In order to do this, index.html has to be the template that is first called (it gets parsed, then executed. During execution the import function re-calls parse - before the block and so it is redefined). So you basically have to use a router template to discover the path:

index.html

{{$path := print ((index .Req.URL.Query.p 0) | trimSuffix "/") ".md"}}
{{ import $path }}
... the restof index.html ...

It doesn't solve the extends issue. Import only works top down. I've been churning on a nice way to do extends but don't have a good idea. You can parse in an arbitrary order, and then execute the base template as long as you know it's name. I was thinking I could build an extends that works by using the internal text/template/parse package and iterating through the nodes for an {{ extends "path" }} node. Then you parse that file. Then you keep going until there are no more extends, and execute the name of the last file you parsed.

That approach breaks if you have multiple layers: Template 1 extends Template 2, which imports Template 3 which extends Template 4. I don't think the solution above would handle extends within import. You'd probably get really confusing results.

I think that could be fixed by also treating import in the pre-process way and using the internal parse nodes to build the template tree and find the base.

I built a quick example using the internal parse package to find the extends. The result of parse.Parse can be manually added to a ParseTree of a template...means we only have to parse once.

Anyways. It's probably close enough that you can't import a template that extends another template. I'll try working on the extends issue for another PR.

@mholt
Copy link
Member

mholt commented Sep 17, 2021

Ok, gotcha. That sounds good.

What was the reason for changing render to import? I'm a little nervous it sounds too similar to include. We can keep import if we want to, but we should have a clear distinction between the two.

@rockorager
Copy link
Contributor Author

Import for two reasons:

  1. It more closely describes what it's doing, ie importing the defined blocks from another file
  2. It aligns with the import in regular go, as in it should be at the top of the file to work properly

render seemed better when you called a template at the spot you wanted it, ie "render this thing and put the output right here"

Agreed it's close to include...I'm good either way. Though the usage is similar to nunjucks:

include pulls in other templates in place.

vs

import loads a different template and allows you to access its exported values.

@mholt
Copy link
Member

mholt commented Sep 17, 2021

Ah, gotcha. Technically though, import could be used on any file, right? It doesn't have to consist solely of a {{ block }} action.

That's fine, but we just need to be sure to document the nuances here. Would you be able to update the godoc comments where the template actions are all documented so that it describes this change and the nuances? Thanks!

@rockorager
Copy link
Contributor Author

That's right, and then to call where to render any doc you would reference it by it's path in block or template.

post.html

Random doc

index.html

{{ import "post.html" }}
{{ template "post.html" }}

include works better for this type of usage I think.

Yep, I'll update the docs and push a new commit!

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much for iterating on this. Let's give it a try.

@mholt mholt merged commit 5fda961 into caddyserver:master Sep 17, 2021
@mholt mholt removed the under review 🧐 Review is pending before merging label Sep 17, 2021
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