Set global variables using nunjucks syntax

[LamJiuFong]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• Feature addition or enhancement
• Code maintenance
• DevOps
• Improve developer experience
• Others, please explain:

Fixes #2302

Overview of changes:
Prepend the content of variables.md to all files before calling nunjucks to render the files

Anything you'd like to highlight/discuss:
Nunjucks does not expose its parser as part of its public API. We can still access its internal nunjucks.parse function but it only allows us to parse a Nunjucks template string into an abstract syntax tree (AST), which represents the structure of the template. Hence we have to implement our own parser to parse various syntax of the different use cases of {% set %}.

Another approach to this issue is to prepend the content of the variables.md to all the files.
Compared to the parser method above, this method accommodates all use cases of {% set %}. This is because we are using nunjucks' parser while rendering the files.

We can also import files using {% ext file = "filepath" %} and use the file globally

Some restrictions have been commented below

Testing instructions:
Commented below

Proposed commit message: (wrap lines at 72 characters)
Support nunjucks syntax for global variables

---

Checklist: ☑️

• Updated the documentation for feature additions and enhancements
• Added tests for bug fixes or features
• Linked all related issues
• No unrelated changes

---

Reviewer checklist:

Indicate the SEMVER impact of the PR:

• Major (when you make incompatible API changes)
• Minor (when you add functionality in a backward compatible manner)
• Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

• To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

• what was the old feature that was made obsolete
• any replacement feature (if any), and
• how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

[EltonGohJH]

@LamJiuFong
I would prefer it if it is in the same file as that is sort of the problem highlighted that we want nunjucks to be supported in variables.md. I am wondering whether it is possible to remove the variables before processing it with nunjucks? Or maybe not rendering it in the first place.

[yucheng11122017]

Yeah I agree with @EltonGohJH.
Plus the original discussion disagrees with the implementation detail of prepending the file to each of the files and I do think it is a bit hacky to do it this way.

[LamJiuFong]

Hi @EltonGohJH @yucheng11122017

Thanks for your suggestions! I have updated my code accordingly

I would prefer it if it is in the same file as that is sort of the problem highlighted that we want nunjucks to be supported in variables.md. I am wondering whether it is possible to remove the variables before processing it with nunjucks? Or maybe not rendering it in the first place.

My current approach is to comment out every <variable> so that nunjucks won't render them
By doing this, we are now able to support nunjucks set in variables.md

This method (commenting out the variables) comes with one restriction:
This works (original version):

<variable name="node_version_number">16</variable>
<variable name="node_version">v{{ node_version_number }}</variable>

But the following doesn't work:

{% set node_version_number = 16 %}
<variable name="node_version">v{{ node_version_number }}</variable>

In short, {% set %} and does not work together

Yeah I agree with @EltonGohJH. Plus the original discussion disagrees with the implementation detail of prepending the file to each of the files and I do think it is a bit hacky to do it this way.

I think it is tricky to implement our own parser to collect all the variables because there are many use cases to consider eg. different data types, math operations, applying functions etc. Our own parser might not be able to support more complex situations though

[LamJiuFong]

Restrictions that we need to document in the User Guide:

1. {% set %} does not work well with <variable> as stated in the previous comment, but their own use cases are preserved
2. Priority order: <variable> > {% set %} in local files > {% set %} in variables.md
   Hence we need to change the following:

The original priority relationship <variable> > {% set %} in local files is still preserved in this case

I personally think that these trade-offs are okay? Since they preserve the original features, in other words, there is no change to users who do not use the new feature

[LamJiuFong]

Testing instructions (Some sample data):

Can add these variables to variables.md:

{% set a = 123 %}
{% set b,c = 100 %}
{% set items = [99, 100, 101, 102] %}

{% set subtotal = 5 %}
{% set taxRate = 0.10 %}
{% set totalTax = subtotal * taxRate %}
{% set totalAmount = subtotal + totalTax %}

{% set hi = "hi" | upper %}

{% ext studentScoreboard = "userGuide/syntax/extra/scoreboard.json" %}

and add use them anywhere, for example:

{{ b }}
{{ c }}

{% for i in items %}
{{ i }}
{% endfor %}

{% ext studentScoreboard = "userGuide/syntax/extra/scoreboard.json" %}

Student Number | Score | Rank
:----- | :-------: | ----
{% for student in studentScoreboard.students -%}
{{ student.number }} | {{ student.score }} | {{ student.rank }}
{% endfor %}

<small>Last updated at {{ studentScoreboard.lastUpdated }}</small>

[yucheng11122017]

Sorry for the late review @LamJiuFong! Could you add some functional test cases so we have a better idea how this should work?

[yucheng11122017]

Why is there this change in the test_site?

[yucheng11122017]

Why is there a need to do this? Isn;t the variable way of declaring already working?

[EltonGohJH]

@LamJiuFong correct me if I am wrong.
I thinkis trying to comment out the variables so that it will not be rendered as stated earlier. As what he is trying to do is to put nunjunks variables on top of the template then render it. To load the nunjucks variables.

If variables is not commented, then it will be rendered.

[LamJiuFong]

@EltonGohJH You are right, it is to prevent the variables from being rendered

[EltonGohJH]

I personally think that it is okay for {% set %} does not work with .

But I feel that this solution is a bit too hacky for my liking. Moreover, the testcases changed? @LamJiuFong Do you know why did the testcase changed as pointed out by @yucheng11122017

[Tim-Siu]

I took a look at the PR and wanted to share a few thoughts on prepending the variables.md content to all files before rendering with Nunjucks.

While it might work, there are a few potential downsides to keep in mind:

1. It could impact performance, especially for larger files or projects with many files.
2. If a file already has Nunjucks syntax or comments, I am not sure if prepending the content will lead to conflicts.

Instead of modifying individual files, I also support configuring the Nunjucks environment to load variables.md during initialization. That way, the variables will be available to all templates without needing to modify each file.

Another option could be using Nunjucks' addGlobal method or creating a separate config file for VariableRenderer-specific variables.

Overall, while the proposed solution might work, it goes against the principle of separating concerns. It's generally better to keep variable declarations centralized and configure the rendering environment appropriately for cleaner code organization and maintainability.

Let me know if you have any other questions or thoughts on this!

[LamJiuFong]

1. It could impact performance, especially for larger files or projects with many files.

I agree with this, I think this is part of the tradeoff. I think we can definitely improve the current proposed solution

2. If a file already has Nunjucks syntax or comments, I am not sure if prepending the content will lead to conflicts.

I couldn't think of any case where conflicts may happen, do you have any ideas?

Instead of modifying individual files, I also support configuring the Nunjucks environment to load variables.md during initialization. That way, the variables will be available to all templates without needing to modify each file.

I am not sure how to achieve this, do you have any idea in mind?

Another option could be using Nunjucks' addGlobal method or creating a separate config file for VariableRenderer-specific variables.

I think the issue with using addGlobal(name, value) is how we extract the names and values from a file containing {% set xxx = xxx %} if we are not creating our own parser to parse the file.

Overall, while the proposed solution might work, it goes against the principle of separating concerns. It's generally better to keep variable declarations centralized and configure the rendering environment appropriately for cleaner code organization and maintainability.

Yup, I agree on this, will work on it!

[LamJiuFong]

@EltonGohJH @yucheng11122017
I have investigated the issue in the test file, I think the issue is caused by the renderString method that I'm using to render the contents.

1. Firstly, the string that I pass into the renderString method contains a lot of \n, hence there is some newlines added in the test file. Eg
   

However, the newlines does not affect the rendering of the HTML files

2. I think the issue here is possibly due to renderString ignoring whitespaces, that's why there is no
   in the output.

We should not allow this as it affects the rendering output of the HTML files

To add on, there are two ways to render the content - renderString(string) or render(file) (the original implementation)
These are the problems faced using both methods:

1. renderString: Takes a string as input and renders the string, so we create a new string by combining the content from variables.md and the target file, but this method will have issues with whitespaces/newlines as shown above
2. render: Takes a file as input and renders the file, we need to create a new file that actually prepends variables.md to the target file, this method will cause a lot of overhead and will slow down the app in general

[LamJiuFong]

As I could not find a way to solve this issue, I am thinking of closing this PR and summarise all the findings/alternatives that I have considered for future developers' reference, what do y'all think?

[EltonGohJH]

@LamJiuFong
Sure. You can close it and report your findings!