-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
text/template, html/template: add ExecuteFuncs #54450
Comments
It isn't clear what you are proposing. Does the argument function map replace the complete set available, does it replace only those in the template, or is it just a place to look first? You don't say what the function signatures are, either. Please edit the proposal to make it clearer. |
I hope that I've made this clearer for you. PR #54432 hopefully shows the intended API in practice. |
A further optimization in #54432 would be to only check and convert the overlay functions in to a valueFunc when the function is called instead of beforehand. |
Thanks, that is clear now. The new names are cumbersome (but could be changed), while thought needs to be given to whether this is the right approach. |
I agree that they're slightly cumbersomely named but they are consistent with naming practices elsewhere in std. Another option could be to make them There is genuinely a need to provide funcmaps at time of execution differently from those used at parsing, and whilst such funcmaps are related - certainly the parsing funcmap constrains what types of funcs and the names of funcs - they're not the same. This is recognised in the current API by having In a lot of ways the naturality of execution funcmaps can be seen in how simple it is to implement. Apart from a small difficulty regarding changing the provided funcmap in to a valuemap there is almost nothing to do to implement it. Now, how else could this be implemented? The suggestion of That leaves the question of is there a way to expose execution state outside of the current functions? Well... The current API means that a |
I should add that the slight distaste in the naming of the go API here should be balanced against the substantial cumbersomeness the current API is imposing on the template API, (and the impracticality of the suggested workaround). A codebase using multiple templates is likely to have only one or two places where The currently suggested workaround of using When balancing these factors, although these method names are slightly cumbersome, it is likely that only one of these 4 methods will be used only once or twice in a codebase. Not having them is making templates incredibly cumbersome and error prone. This suggests really that they would be a net reduction in cumbersomeness. |
@bep, would this simplify the implementation of Hugo? Being able to call a |
Dear golang team have there been any further thoughts about this? |
@carlmjohnson I don't understand this issue very well (I have only skimmed it). In Hugo we have a fork of the execution part of Go's template packages with some small adjustments (we have a script that keep the 2 in sync); one of them is a |
Forking the execution code is part is something I am trying to avoid... |
Is this proposal still under discussion? I agree with zeripath that the customized Without this approach, I guess the developers should do:
According my test, this approach is not feasible, for a site with 400 templates:
|
@robpike I also think this is the right approach, is there any more concern? |
I still believe that #38114 is the path forward here. The fact that no one has had time to do it does not necessarily imply it is difficult or impossible, just low priority. |
The
Without official support, there are a lot of work to do, because the sub-templates share the internal state:
|
This proposal has been added to the active column of the proposals project |
I have talked about this before, without seeing much enthusiasm, but I'll repeat the bullet points. Hugo used to do a clone of the full template set for each language to be able to have language specific template functions. This was ... not great, as many have mentioned above. Now we have a "soft fork" of Go's template packages where we have made some slight adjustments to the execution flow. The relevant part in this discussion could be illustrated with the 2 interfaces below: // Preparer prepares the template before execution.
type Preparer interface {
Prepare() (*Template, error)
}
// Executer executes a given template.
type Executer interface {
ExecuteWithContext(ctx context.Context, p Preparer, wr io.Writer, data any) error
} With the above, |
@bep would the proposed API help hugo get rid of the "soft fork" of Go's template? (or at least remove some customization) I would welcome such a change to get rid of the |
# Background Golang template is not friendly for large projects, and Golang template team is quite slow, related: * `golang/go#54450 Without upstream support, we can also have our solution to make HTML template functions support context. It helps a lot, the above Golang template issue `#54450` explains a lot: 1. It makes `{{Locale.Tr}}` could be used in any template, without passing unclear `(dict "root" . )` anymore. 2. More and more functions need `context`, like `avatar`, etc, we do not need to do `(dict "Context" $.Context)` anymore. 3. Many request-related functions could be shared by parent&children templates, like "user setting" / "system setting" See the test `TestScopedTemplateSetFuncMap`, one template set, two `Execute` calls with different `CtxFunc`. # The Solution Instead of waiting for upstream, this PR re-uses the escaped HTML template trees, use `AddParseTree` to add related templates/trees to a new template instance, then the new template instance can have its own FuncMap , the function calls in the template trees will always use the new template's FuncMap. `template.New` / `template.AddParseTree` / `adding-FuncMap` are all quite fast, so the performance is not affected. The details: 1. Make a new `html/template/Template` for `all` templates 2. Add template code to the `all` template 3. Freeze the `all` template, reset its exec func map, it shouldn't execute any template. 4. When a router wants to render a template by its `name` 1. Find the `name` in `all` 2. Find all its related sub templates 3. Escape all related templates (just like what the html template package does) 4. Add the escaped parse-trees of related templates into a new (scoped) `text/template/Template` 5. Add context-related func map into the new (scoped) text template 6. Execute the new (scoped) text template 7. To improve performance, the escaped templates are cached to `template sets` # FAQ ## There is a `unsafe` call, is this PR unsafe? This PR is safe. Golang has strict language definition, it's safe to do so: https://pkg.go.dev/unsafe#Pointer (1) Conversion of a *T1 to Pointer to *T2 ## What if Golang template supports such feature in the future? The public structs/interfaces/functions introduced by this PR is quite simple, the code of `HTMLRender` is not changed too much. It's very easy to switch to the official mechanism if there would be one. ## Does this PR change the template execution behavior? No, see the tests (welcome to design more tests if it's necessary) --------- Co-authored-by: silverwind <me@silverwind.io> Co-authored-by: Jason Song <i@wolfogre.com> Co-authored-by: Giteabot <teabot@gitea.io>
Hello ~~ kindly ping .... @rsc is there any progress (weekly review)? Update: if the name is somewhat "cumbersome", it could only keep the "ExecuteFuncMap" since "ExecuteTemplateFuncMap" is only a wrapper. "ExecuteFuncMap" is good enough and in most cases developers only need it. |
@oliverpool It would be a step forward ... I can understand why the Go std library team would be a little worried about changing the API, but it should be easy to keep to old while getting a, in my eye, much more performant and clean design where the concerns of parsing and execution are clearly separated. |
Change https://go.dev/cl/510738 mentions this issue: |
I spent a while looking into optimizing Clone, and while I've convinced myself it can be done with significant changes, it seems that not making those changes is the better part of valor. I also think there's no need for ExecuteTemplateFuncMap, since ExecuteTemplate is just a helper wrapper around Lookup+Execute and can be left alone. Finally I think we can shorten ExecuteFuncMap to ExecuteFuncs, especially since the Template method is Funcs. So this proposal shortens to just adding ExecuteFuncs in the two packages, which I've sent CL 510738. I've retitled it as such. Please try out that CL and see if it does everything you want it to do. Thanks! |
The only question in my mind is: Is it necessary to call The code is a hot path, checking the funcs or not, the result could be the same. Suppose a developer passes "funcs" into
Eventually there seems no different, while "without createValueFuncs" is more performant |
Please comment on the CL for that implementation detail. I don't think it affects the proposal. Thanks! |
@bep, does this look good to you? Can you try it in Hugo and see if it lets you remove one of the divergences? |
@rsc so, the current set of execution related methods duplicated in both
With the addition of
When you eventually decide you want to add
Or something. I have read your recent blog articles about storing state in control flow etc., but reading the above, I would say that it would make perfect sense to introduce a new exported struct type that would be responsible to execute a given template. This new struct would hold the funcs (which could be initialized once at construction time) and you could get a API ala:
Or something. This would also support most future requirements that may pop up in this area. |
@bep It seems questionable to me, that you assume there will be And even if we decided we need a native way to call context functions, I don't see why it couldn't take the form of a single So, no, I honestly don't see that. |
@Merovius My The thing is, state doesn't vanish just because you add it as a function argument (e.g.
I feel like every proposal about improving something in any of these template packages end up with making the least amount of change to get something working, often ending up with yet another unforeseen data race etc.
Se my "the state doesn't vanish" argument above. I'm not sure how global state would somehow be better than a struct for this (again, see my comment about introducing unforeseen data races above). |
Do you mean #31107? Because that is closed, ultimately in favor of this one, AIUI.
I agree, for what it's worth. That's why I said that we shouldn't complicate this proposal by assuming we'd also add |
Sure, but we should take into account that for every similar API in Go's stdlib (that does "per request operations", e.g. |
Background: One of the things Hugo can do is to make an HTTP requests in a template. It wouldn’t make sense in a normal HTTP server app, but it makes sense for an application where the user controls templates but not the handlers. If a user makes a file change and then another one, the first request needs to be cancelled. |
If I were going to do ExecuteContext (big if!) I would make that one new function take both the context and the funcs. |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
This proposal is about providing a way to pass a
template.FuncMap
at template execution overriding specific functions within the template.When executing a template using one of new methods, then the provided
template.FuncMap
will be checked first.Summary
Reasons:
data any
parameter and this is suboptimal for contextual rendering elements such as web nonces likecsrf
, or translation functions.Clone()
,Funcs()
and thenExecute()
but this is slow and optimizing this appears very difficult. (text/template: optimize Clone+Funcs+Execute sequence #38114) It would require significant changes throughout the code and even if parent templates could be changed to allow inheritance, It would requires cloning every template that could be called in advance.Advantages:
Detailed rationale
Sometimes data needs to be rendered differently based on the context it is rendered in.
The two most common examples anti-CSRF tokens and locales.
Although currently these can be added to the data struct provided when rendering - these are not strictly data and it significantly complicates rendering especially when considering subtemplating.
Proposed API
I propose that the following methods are added to
text/template.Template
and tohtml/template.Template
text/template
:html/template
:Calling these methods would execute the template as per
Execute
andExecuteTemplate
with the only difference being that when a template calls a function the providedfuncs template.FuncMap
would be checked first. In this wayfuncs
can be considered a map of function overrides for template execution and acts similarly to the calling theFuncs(...)
method on the template but would also override functions in templates called by the current executing template.Examples
Current Status
Let us assume we have a function with the following signature:
In this example this function takes a
key
and provides the translated value for it.Clearly, this cannot be compiled into shared templates as the locale will be different on a per request basis.
So assuming we don't want to have multiple compiled templates per locale we will need to pass this in as data. Let's say as
.Tr
.This leads to:
But... what about when ranging over a set of items? Now,
.
will refer to an item in the range of items so we need use$
...If we forget we will get a runtime error. So we won't know that it's a problem until it's run.
But... what if we want to reuse that range block as a subtemplate or even just extract it out because it is becoming unwieldy? Ideally we'd want to do:
That of course doesn't work because we lose access to
$
. So now we need to do something horrible like:and then in our subtemplate refer to
.Item
and.Tr
(or.root.Tr
). This means that we need to now know that we're in a subtemplate and every time we call the subtemplate we need to do a similar hack.Again if we forget to add the correct prefix we'll get a runtime error but only at time of render.
Why isn't
template.Clone()
template.Funcs()
template.Execute()
sufficient?The problem here is multifactorial,
Clone()
isn't very lightweight and requires locks,Funcs(...)
also requires locks and every template would need to be cloned and its funcs updated in case when you execute the template it calls another template.The referenced issue has been around for over 2 years and it is likely that improving the efficiency of this route significantly is not possible without significant refactors.
Instead, the proposed API makes for a clear and simple implementation with a minimal changeset.
Proposed
The proposed API allows for much simpler templating. The
Tr
function would be passed in within an overrideFuncMap
e.g.:Then the downstream templates look like:
And the
Tr
function is available everywhere without having to determine whether it needs a.
, a$
or even a$.root
. (Or even if it's not been passed in to this subtemplate.)Compatibility
The proposed functionality is completely backwards compatible.
Implementation
The proposal is easy to implement and has been implemented as #54432.
The text was updated successfully, but these errors were encountered: