Support variables to be defined in JSON

[crphang]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

Resolves#319

• [X] Enhancement to an existing feature

What is the rationale for this request?

Allow variable to be defined in JSON. This allows us to define variable in different HTML fragments such as:

What changes did you make? (Give an overview)

Added an additional logic to handle variable defined in JSON.

Provide some example code that this change will affect:

<variable type="json">
{
"front": "<div> front",
"back": "back </div>"
}
</variable>
{{ front }} {{ back}}

Is there anything you'd like reviewers to focus on?

One of the attempts before relying on a common syntax like JSON was to enable nunjucks variables. However, there is no easy way to retrieve variables from nunjucks because of it's design: mozilla/nunjucks#329.

Testing instructions:

npm run test

Proposed commit message: (wrap lines at 72 characters)

Support variables to be defined in JSON.

[alyip98]

LGTM

[openorclose]

Is there any other place where <variable> defines multiple variables?

Trying to read

<variabletype="json">
{
"front": "<div> front",
"back": "back </div>"
}
</variable>

sounds like "I'm creating a variable of type 'json' " to me, and not "I'm creating multiple variables whose names and values are defined in this json object".

[damithc]

How about this?

<variables>
{
"front": "<div> front",
"back": "back </div>"
}
</variables>

variables by default should be json syntax. Braces can be made optional (?)

<variables>
"front": "<div> front",
"back": "back </div>"
</variables>

• main benefit of the feature: can be used to define multiple variables in one place.
• a side benefit: can be used to put non-well-formed values into variables
• caveat: clunky if the value is long and has a lot of symbols

[acjh]

How about introducing a variables.json file for arbitrary strings?
I think this actually makes sense for a lot of those in nus-cs2103/website-base's variables.md.

I suspect why we had variables.md was to support valid HTML.
The example given would actually be interpreted in HTML as:

<variabletype="json">
" { "front": ""
<div> front", "back": "back </div>
"" } "
</variable>

A limitation is that we have to define an order for which file is processed first (likely variables.json), which restricts which file can use variables in the other.
@damithc Is this limitation compatible with your use case?

[damithc]

@damithc Is this limitation compatible with your use case?

variables.json would have the global scope right? While having one would be good, it doesn't serve cases where the variable needs to be in local scope. e.g., an ugly partial html fragment that needs to be repeated several times in a particular page but is not used anywhere else.

[acjh]

(Maybe I forgot something) but isn't variables.md scoped similarly?

[damithc]

(Maybe I forgot something) but isn't variables.md scoped similarly?

It is. I thought this new feature is meant to be used in both variables.md and as page variables. @crphang does it work like that?

[crphang]

"I'm creating a variable of type 'json'

True. Perhaps renaming the attribute might help such as from=json. Alternatively we could interpret content on the fly, by using value produced by JSON parse if it is valid and defaults to regular string expressions.

It is. I thought this new feature is meant to be used in both variables.md and as page variables.

Yes that was the intention. However, since the file is still regarded as a .md and we are retrieving the content from cheerio, we might not be completely immune of issues regarding interference of syntax even though the tests I have added suggests that it alright.

[ang-zeyu]

If the aim is to allow any content inside <variable>, how about adding it to the list of special tags?
Dosen't solve the second part of the issue though, so maybe we could add this to that

How about introducing a variables.json file for arbitrary strings?

[acjh]

even though the tests I have added suggests that it alright.

@crphang The test that you have added for HTML happens to close itself.
Try swopping the order or only having the one with the opening tag.

[crphang]

Letting variables be defined as Json within the page will cause issue if the HTML fragments are not self-closing as @acjh suggests.

I took another approach to allow user to json variables define in separate json file.

1. It supports variables.json as a separate file, but will still be backward compatible variables.md as well as page variables defined inline. This approach supports both page variables and global user defined variables.
2. I also added this in initialization, to make it easy for new users to choose between defining variables inline or in a separate json file.

We could further add in user guide in tips and tricks section about allowing malformed html fragments in json variables if user requires.

[damithc]

Remember to update user docs too.

[crphang]

Updated user guide. Ready for review.

[crphang]

@yamgent could you help with the review of this? 🙇

[crphang]

@marvinchin could you help review this? 🙇‍♂️

[marvinchin]

Couple of suggestions!

[crphang]

@marvinchin thanks for reviewing! Updated with the suggested changes 😄

[marvinchin]

Do you feel it makes sense to add this into the abstracted function too?

const variableValue = nunjuckUtils.renderEscaped(
nunjucks, md.renderInline(variableElement.html()), {
...importedVariables, ...pageVariables, ...userDefinedVariables, ...includedVariables,
});

[marvinchin]

Or perhaps, would it make sense to define an inline function? This way we can get the benefits of abstraction while still being able to access the variables we need directly. We could do something like:

const processVariable = (variableName, rawVariableValue) => {
const otherVariables = { ...importedVariables, ...pageVariables, ...userDefinedVariables, ...includedVariables };
const variableValue = nunjckUtils.renderEscaped(nunjucks, rawVariableValue, otherVariables);
if (!pageVariables[variableName]) {
pageVariables[variableName] = variableValue;
Parser.VARIABLE_LOOKUP.get(fileName).set(variableName, variableValue);
}
}

[crphang]

@marvinchin Thanks for the suggestion, I refactored to make the logic cleaner as suggested. 👍

[marvinchin]

LGTM 🎉