Uh oh!
There was an error while loading. Please reload this page.
Fix nunjucks path resolving - #1263
Conversation
damithc
commented
Jun 28, 2020
This will be a big step forward if we manage to pull it off. 👍 |
ang-zeyu
commented
Jun 28, 2020
It's pretty much ready, just need to update some more test files. I'm putting it on hold as we are doing quite a bit of directory restructuring as suggested by @acjh here #1253 (comment). Would be easier to review then.
The next step would be layouts such as header/footer/expressive layouts and such, I'm not sure if the current behaviour is intended though (layout files from the root site can currently be used in subsites, unlike variables) All of this should be able to be easily achieved by building the sites individually (the work so far still being necessary even if so though), and "blocking parent sites from building sub site pages". we should probably flesh out the requirements for sub sites first in an issue or document the current behaviour in ug (if intended) |
marvinchin
left a comment
There was a problem hiding this comment.
This looks fine to me! Just to check - I assume this has been tested on sites with nunjucks (2103T maybe?)? I don't see anything that would change existing working behaviour, but just want to be sure 🙂
Just a suggestion regarding structuring commits - I think it would be nice to have renaming variablePreprocessor -> variableProcessor to be in its own commit. This way the reviewer can filter the commits to focus only on the material changes.
Nunjucks' features that use paths resolve from the specified template
directory by design.
In MarkBind's case, the template directories are the respective root
paths of the various sites and subsites of the project.
Hence, a separate nunjucks environment with the template directory
configured as such is needed for each site or subsite.
Let's introduce a wrapper abstraction for this, VariableRenderer, to
wrap over such a singular nunjucks environment.
The VariableRenderer instances will be managed by VariableProcessor
(previously VariablePreprocessor), which already has a framework set up
to render variables belonging to each site or subsite.
In addition, {% raw %} tags inside a layouts file do not work due to
two nunjucks passes being made.
Let's revert the removal of the helper functions to keep {% raw %} tags
in the first pass to fix this, while searching for a solution that
would allow a single pass for layouts.ang-zeyu
commented
Jul 23, 2020
yup!
That makes sense, my bad 😅 I reseparated it - although there's some renaming in the first commit as well. This should be tremendously easier to look at in the history still though. I also moved the variable rendering portion in Also more clearly separates the variable rendering from the "including" part of the process. (retested with 2103 site again too) |
| Render {{ MAIN_CONTENT_BODY }} and {% raw/endraw %} back to itself first, | ||
| which is then dealt with in the call below to {@link renderSiteVariables}. | ||
| */ | ||
| .then(result => this.variableProcessor.renderPage(layoutPagePath, result, { |
There was a problem hiding this comment.
variable rendering is moved out of includeFile, also in page.generate() / resolveDependency()
| * @param content to render | ||
| * @param highestPriorityVariables to render with the highest priority if any. | ||
| * This is currently only used for the MAIN_CONTENT_BODY in layouts. | ||
| * @param keepPercentRaw whether to reoutput {% raw/endraw %} tags, also used only for layouts. |
There was a problem hiding this comment.
Now that variable rendering is moved out of include file, it seems clearer that there is some specific behaviour for layouts. Do you think it's worth having a renderLayout function instead of using a general purpose renderPage?
There was a problem hiding this comment.
that makes sense! I think there's quite a bit of opportunity for refactor regarding layouts though, apart from this. (site nav, page layouts, header, footer takes up a sizeable portion of Page.js) Perhaps we could defer it to that pr?
There was a problem hiding this comment.
That sounds fine too, let's fix that in the next PR then 🚀
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] Bug fix
Fixes#931
What is the rationale for this request?
What changes did you make? (Give an overview)
To be filled
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Fix nunjucks path resolving
Nunjucks' features that use paths resolve from the specified template
directory by design.
In MarkBind's case, the template directories are the respective root
paths of the various sites and subsites of the project.
Hence, a separate nunjucks environment with the template directory
configured as such is needed for each site or subsite.
Let's introduce a wrapper abstraction for this, VariableRenderer, to
wrap over such a singular nunjucks environment.
The VariableRenderer instances will be managed by VariableProcessor
(previously VariablePreprocessor), which already has a framework set up
to render variables belonging to each site or subsite.
In addition, {% raw %} tags inside a layouts file do not work due to
two nunjucks passes being made.
Let's revert the removal of the helper functions to keep {% raw %} tags
in the first pass to fix this, while searching for a solution that
would allow a single pass for layouts.