-
-
Notifications
You must be signed in to change notification settings - Fork 8.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
refactor(v2): precompile ETA templates #3238
Conversation
|
||
function renderSSRTemplate(data) { | ||
const compiled = getCompiledSSRTemplate(); | ||
return compiled(data, {...eta.defaultConfig, ...customEtaConfig}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebrelbug I found this part a bit confusing. Why do I need to provide some config in the compiled template. Can't it uses the partial config I set during compilation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this looks weird to me to expect a full config here, why not partial + you merge to defaults in the lib directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @slorber, good question. You need to provide a config to the compiled template function because the config object holds partials, filters, and other utility methods Eta uses while actually rendering the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do accept partial configs in compile
and render
. Those functions merge passed-in config options with Eta's default configuration.
It's better for performance, though, to not have to merge the config every time we call the compiled function. Plus, in most cases it would be redundant (most users call render()
to render a template, which already merges the config before calling the compiled template function and returning the result) 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did realize that in this case, we can just call the compiled function with eta.defaultConfig
. The {rmWhitespace: true}
option is applied only when the template is compiled, and doesn't affect the template's rendering.
Deploy preview for docusaurus-2 ready! Built with commit bab2dd6 |
Hi @slorber, it looks like you're right: I forgot to set the |
When Eta compiles a template, it returns a function which can be called with data and a valid configuration object. Since the templates won't change, I might suggest doing something like this: // createRedirectPageContent.ts
// We don't have to generate this every time because the template won't change
let compiledTemplate = eta.compile(redirectPageTemplate.trim())
function renderRedirectPageTemplate(data: object) {
return compiledTemplate(data, eta.defaultConfig);
}
// We could memoize the renderRedirectPageTemplate function,
// since it will always return the same result when called with the same data
// Since compiled template functions are very fast, though,
// I'm not sure if it would bring a noticeable performance gain. // docusaurus-theme-search-algolia/src/index.js
let compiledOpenSearchTemplate = eta.compile(openSearchTemplate.trim())
function renderOpenSearchTemplate(data) {
return compiledOpenSearchTemplate(data, eta.defaultConfig);
} // docusaurus/src/client/serverEntry.js
let compiledSSRTemplate = eta.compile(ssrTemplate.trim(), {rmWhitespace: true})
function renderSSRTemplate(data) {
// We can render with eta.defaultConfig, because {rmWhitespace: true} only affects the template
// during compilation
return compiledSSRTemplate(data, eta.defaultConfig);
} |
Note that we could memoize Since the template functions basically just perform string concatenation, however, I'm not sure this would bring an additional noticeable performance increase. |
this means the template is compiled at plugin initialization time. I don't necessarily want the template to be compiled early in the process, particularly if it's only used in the postBuild step, that's why I use the memoize in code, to compile it lazily, yet cache for further uses. I don't want to memorize the render() call because it will be very often called with different args, and the memorize cache would grow, consume memory, and mostly produce cache misses. Does this PR looks good to merge or do you see things to improve? |
|
||
function renderSSRTemplate(data) { | ||
const compiled = getCompiledSSRTemplate(); | ||
return compiled(data, {...eta.defaultConfig, ...customEtaConfig}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change this to return compiled(data, eta.defaultConfig);
import {memoize} from 'lodash'; | ||
|
||
const customEtaConfig = { | ||
name: 'ssr-template', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the name
option: it's used only for Eta's built-in caching inside render() and compile(), which we're bypassing to render.
That makes sense. I just suggested a few additional changes, then I think it looks good! 👍 |
Thanks, just made the last changes and will merge asap |
Motivation
ETA has a caching system but reading the doc, it's not totally clear if we leverage it correctly (+ current code did .trim() on each SSR page: useless work)
I just precompiled the templates lazily in a memoize call to see if it can improve build times.
cc @nebrelbug if you can review and tell me what you think. I'm not sure we leveraged Eta caching anyway (despite providing a template name), as the default cache option is not documented here: https://eta.js.org/docs/api/configuration
Do you think this PR has an impact?