Remove dependency on EJS

[le0tan]

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

• [X] Other, please explain:

Remove unnecessary dependency of ejs.

Resolves#988

What is the rationale for this request?

It seems that MarkBind is using both nunjucks and ejs as templating engines. However ejs is only used to render page.ejs. It's probably a bit unnecessary to keep ejs to use it only once while it can be replaced with nunjucks?

What changes did you make? (Give an overview)

Replace page.ejs with page.njk and change the initial template rendering from EJS to Nunjucks.

Provide some example code that this change will affect:

Ideally this shouldn't change any behavior.

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

Is variables still working as expected? As I changed one test on variables referring to another variable.

Testing instructions:

npm run testwin should do.

Proposed commit message: (wrap lines at 72 characters)

Remove dependency on EJS

MarkBind is using two templating engines (i.e. EJS and Nunjucks) while
Nunjucks is essential to the functionality since it's user accessible,
EJS seems a bit redundant and can be fully replaced by Nunjucks.

• Re-write page.ejs with Nunjucks grammar
• Make necessary changes to remove dependency on EJS
• Change test on nested variable reference as initial Nunjucks renderer
  is set to not auto-escape

[le0tan]

This is the failed test case:

Site resolves variables referencing other variables
expect(received).toEqual(expected) // deep equality
Expected: "<span style="color: blue">Blue text</span>"
Received: "<span style=\"color: blue\">Blue text</span>"
367 | .replace(/>/g, '&gt;');
368 | expect(root.level3).toEqual(expectedTextSpan);
> 369 | expect(root.level4).toEqual(expectedTextSpanEscaped);
| ^
370 | });
371 | 372 | test('Site read correct user defined variables', async () => {
at Object.<anonymous> (test/unit/Site.test.js:369:23)

@openorclose do you have any ideas?

[openorclose]

This is the failed test case:

Site resolves variables referencing other variables
expect(received).toEqual(expected) // deep equality
Expected: "<span style="color: blue">Blue text</span>"
Received: "<span style=\"color: blue\">Blue text</span>"
367 | .replace(/>/g, '&gt;');
368 | expect(root.level3).toEqual(expectedTextSpan);
> 369 | expect(root.level4).toEqual(expectedTextSpanEscaped);
| ^
370 | });
371 | 372 | test('Site read correct user defined variables', async () => {
at Object.<anonymous> (test/unit/Site.test.js:369:23)

@openorclose do you have any ideas?

Not sure what the test is testing, but when i tried it on a real site it does seem to produce the non-escaped version...

[yamgent]

From the issue tracker:

It seems that MarkBind is using both nunjucks and ejs as templating engines. However ejs is only used to render page.ejs. It's probably a bit unnecessary to keep ejs to use it only once while it can be replaced with nunjucks?

Yes, technical-wise, there shouldn't be any problem with replacing ejs with nunjucks. Iirc, the main argument back then for using ejs was that since this isn't exposed to the users at all, using ejs was a better choice because it was closer to javascript in terms of design, which is what we developers use.

However, given the current nature of the project + possibility of making maintainability better, I am open to switching out the ejs.

Not sure what the test is testing, but when i tried it on a real site it does seem to produce the non-escaped version...

If nunjucks is set to autoescape mode, the variable will came up empty, which is why the escaping was necessary back then. If this PR has changed the behaviour, such that the render will still be correct even without escaping, then the test should be changed to reflect that (i.e. expect(root.level4).toEqual(expectedTextSpan);).

[le0tan]

Not sure what the test is testing, but when i tried it on a real site it does seem to produce the non-escaped version...

From the description of this test, I suppose my PR is working correctly?

[openorclose]

Not sure what the test is testing, but when i tried it on a real site it does seem to produce the non-escaped version...

From the description of this test, I suppose my PR is working correctly?

Yeah, from what @yamgent commented, since now your nunjucks autoescape is set to false you can just edit the tests to reflect it.

[yamgent]

There are now a lot of blank spaces in the output.

...
<linkrel="stylesheet" href="markbind/fontawesome/css/all.min.css"><linkrel="stylesheet" href="markbind/glyphicons/css/bootstrap-glyphicons.min.css"><linkrel="stylesheet" href="markbind/css/github.min.css"><linkrel="stylesheet" href="markbind/css/markbind.css"><linkrel="stylesheet" href="markbind/layouts/default/styles.css"><linkrel="stylesheet" href="markbind/css/site-nav.css"><linkrel="stylesheet" href="markbind/css/page-nav.css"></head>

You can get rid of the blank spaces in the render by using whitespace control. For example, changing the above to this will (1) remove unnecessary whitespace, and (2) make the align with each other:

{%- if asset.pluginLinks %}
{%- for link in asset.pluginLinks %}
{{ link }}
{%- endfor %}
{%- endif %}

[yamgent]

The format and indentation in the original file src/page.ejs are like that due to the whitespace issue, so if unsure following the original one is the easiest.

[le0tan]

I applied the whitespace control to the entire file. Thanks for the tips!