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

Proof of concept: Template context function #24457

Closed
wants to merge 1 commit into from

Conversation

wxiaoguang
Copy link
Contributor

cc @yardenshoham for #24342

This is a proof of concept of the "template context function".

Now in templates we only need to do:

Locale: {{CtxLocale "home"}}<br>
DateTime: {{CtxDateTime "full" "2001-02-03"}}<br>

TODO:

  1. Refactor the 500 page rendering
  2. Design better name/struct for CtxFunc

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 1, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 1, 2023
@delvh
Copy link
Member

delvh commented May 1, 2023

Phew, I don't really know if I like the proposed approach.
That will cause some nasty import problems, as Go doesn't allow import cycles, and modules/context is pretty much our backbone package…
This means for example that we cannot add any function located inside models or services that needs a context…
I mean, we can circumvent this with offering modular functions, i.e.

type Context {
  …
  loadUser func(ctx Context, <remaining params>)
}

func (ctx *Context) loadUser(<remaining params>) {
  loadUser(ctx, <remaining params>)
}

but that doesn't scale well and is a lot of overhead code to write…

@wxiaoguang
Copy link
Contributor Author

That will cause some nasty import problems, as Go doesn't allow import cycles, and modules/context is pretty much our backbone package…

"import", no worry, the framework can handle them correctly later.

@silverwind
Copy link
Member

Design better name/struct for CtxFunc

I guess we can keep the same names, e.g. Locale. The refactor will be massive in changed lines count, but other than that, trivial via sed/perl replacement 😉

@silverwind
Copy link
Member

Should we merge this POC?

@wxiaoguang
Copy link
Contributor Author

Nope, it's far from complete. It needs #24461 and maybe some other PRs before merging.

@wxiaoguang wxiaoguang marked this pull request as draft May 4, 2023 02:02
lunny pushed a commit that referenced this pull request May 4, 2023
Partially for #24457

Major changes:

1. The old `signedUserNameStringPointerKey` is quite hacky, use
`ctx.Data[SignedUser]` instead
2. Move duplicate code from `Contexter` to `CommonTemplateContextData`
3. Remove incorrect copying&pasting code `ctx.Data["Err_Password"] =
true` in API handlers
4. Use one unique `RenderPanicErrorPage` for panic error page rendering
5. Move `stripSlashesMiddleware` to be the first middleware
6. Install global panic recovery handler, it works for both `install`
and `web`
7. Make `500.tmpl` only depend minimal template functions/variables,
avoid triggering new panics

Screenshot:

<details>

![image](https://user-images.githubusercontent.com/2114189/235444895-cecbabb8-e7dc-4360-a31c-b982d11946a7.png)

</details>
@delvh
Copy link
Member

delvh commented May 25, 2023

Still blocked?

@wxiaoguang
Copy link
Contributor Author

Not blocked by code but blocked by my limited time 😭

@wxiaoguang
Copy link
Contributor Author

Golang team make some progress

text/template, html/template: add ExecuteFuncs https://go-review.googlesource.com/c/go/+/510738

I think the official "context function" support will be available soon.

@wxiaoguang
Copy link
Contributor Author

Wait for the Golang official solution

@wxiaoguang wxiaoguang closed this Jul 22, 2023
@wxiaoguang wxiaoguang deleted the use-ctx-func branch July 22, 2023 04:22
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants