Nunjucks Multilevel Template Inheritance is broken

[j-mori]

• Maizzle Version: 0.4.4
• Node.js Version: 12.13.1

Description:

Inheritance between templates doesn't work.

You can find the feature's documentation here: Nunjucks (https://mozilla.github.io/nunjucks/templating.html#template-inheritance) and Jinja (https://jinja.palletsprojects.com/en/2.10.x/templates/#template-inheritance)

This is caused by the code who extends the layout for all templates.

node_modules/@maizzle/framework/src/generators/output/toDisk.js:60

html = `{% extends "${layout}" %}\n${frontMatter.body}`

Some solutions could be to skip layout inclusion if the template contains {% extends, "layout" setting is explicitly set to false or a setting doNotExtendsLayout is set.

Steps To Reproduce:

Extends a file or a template with another template and build.

[cossssmin]

We could do something like:

html = layout ? `{% extends "${layout}" %}\n${frontMatter.body}` : frontMatter.body

So then you'd set layout: false in the template's Front Matter to prevent this. Or as you mentioned, look for {% extends and do it without user intervention.

Perhaps I'm not thinking this through - can you please share a real-life scenario where you'd need a template to extend another template?

[cossssmin]

Actually, can you please provide an example where it's broken, and how you'd expect it to be output?

[j-mori]

We could do something like:

html = layout ? `{% extends "${layout}" %}\n${frontMatter.body}` : frontMatter.body

So then you'd set layout: false in the template's Front Matter to prevent this.

Just tested out, html = layout ? `{% extends "${layout}" %}\n${frontMatter.body}` : frontMatter.body we ll' do the trick but we should also change toDisk.js:53 let layout = config.layout || config.build.layout into let layout = config.layout && config.build.layout otherwise we can't set layout= false via Front Matter because false we'll be ignored by the || operator.

Or as you mentioned, look for {% extends and do it without user intervention.

I think, in addition, we should look for {% extends ... %} string in template because in nunjucks it's not possible anyway in just a single template extends multiple templates so its a good check to do. Probably a regex like {%\s+?extends.+\s+?%} should work but maybe it'll slow down compile time for big layouts?

Perhaps I'm not thinking this through - can you please share a real-life scenario where you'd need a template to extend another template?

The scenario are various, i could have 2 similar newsletter template with just a couple of differences in few blocks, in this case i could extends my newsletter template and redefine just the blocks with desired changes. With nunjucks this is possible at infinite sub-levels so i could extends a template who extends another template and so on. You could workaround with partials as well but it's just bad to lose a such cool feature who provide huge scalability for a small problem.

[j-mori]

Actually, can you please provide an example where it's broken, and how you'd expect it to be output?

of course, take for example your default template transictional.njk, and wrap the Confirm your email button inside a {% block button %}{% endblock %} block. now create another template transictional_account.njk:

{% extends "src/templates/transactional.njk" %}

{% block button %}Confirm your account{% endblock %}

Now my expectation are to get 2 differents templates: One with "Confirm your email" button and another with "Confirm your account".

But Instead of the "Confirm your account" template i get a blank template becasue maizzle add the layout extensions on top of my tamplate, nullifing my extends line code. Obviously this isn't the best feature case scenario becuse the button could be a compnent with a text parameter, but it's just to demostrate how and why the things breaks

[cossssmin]

Thanks for the explanation, it's clear now.

I'm currently working on updating toDisk() to use the toString() method, to consolidate the codebase and make it easier to write tests in the future. I'd like to release this first (ideally this week or the next), and we can then handle this issue 👍

[j-mori]

Perfect gonna follow the updates cause we are very interest to this framework!

[cossssmin]

Btw you could use Components for your buttons example - just pass in the text. Let me know if you want me to show you an example.

[j-mori]

Yeah i know in that particularly case components are probably a better way to reach the result but they was just simple examples, when it comes to more complex templates where i would like to changes an entire code block instead of just a button (for exeample a column in a template with columns), template inheritance is a much better way to handle the case.

Anyway we should definetly fix this just because template inheritance it's a documented and cool feature that Nunjucks offers and at the moment is not working.

If you want i can create a pull request with the changes mentioned above on the current maizzle version or if you prefer i can wait a couple of week for the new relase

[cossssmin]

Yes, of course, the issue still stands - just wanted to have it mentioned. Would love to see a PR, but yeah please wait until I publish the new release. Thanks!

[cossssmin]

Merged the PR that refactors toDisk(), can you work off of master or do you need me to publish a release? I'd honestly prefer if we publish a v0.5.0 that includes both that and the inheritance fix.

[cossssmin]

Fixed it, should work as expected now. Feel free to reopen if you spot any issues ✌

[j-mori]

Thanks!

[cossssmin]

Sorry, had to roll it back in v0.5.1 because of several issues:

• maizzle serve was getting an undefined layout, so it was not using any
• all templates were using the same (default) layout, even if they extended a different one

I'm open to suggestions :)

[cossssmin]

One possible solution would be to recursively recompile until there's no layout found.

For example, this works:

html = `{% extends "${config.layout}" %}\n${html}`
html = nunjucks.renderString(html, { page: config, env: options.env, css: compiledCSS })

while (fm(html).attributes.layout) {
  const front = fm(html)
  html = `{% extends "${front.attributes.layout}" %}\n{% block template %}${front.body}{% endblock %}`
  html = nunjucks.renderString(html, { page: config, env: options.env, css: compiledCSS })
}

Biggest downside is we'd need to hardcode the block identifier for inherited Templates. So if the Layout they eventually extend doesn't use that identifier, an empty <body> would be output.

For example, this works:

<!-- src/layouts/default.njk -->
<body>
  {% block template %}{% endblock %}
</body>

<!-- src/templates/first.njk -->
---
layout: src/layouts/default.njk
---

{% block template %}
  Hello, {% block firstname %}Jane{% endblock %}
{% endblock %}

<!-- src/templates/second.njk -->
---
layout: src/templates/first.njk
---

{% block template %}
  Hello, {% block firstname %}John{% endblock %}
{% endblock %}

But if someone prefers to name their block in the Layout differently, say {% block myTemplateContent %}, then second.njk would contain an empty <body>, since we would have {% block template %} hardcoded and that wouldn't exist in the default.njk Layout.

Unfortunately, since you can have as many {% block %} tags as you want, and there's no mandatory order to write them in, we can't get the identifier name either - it can very well be that the first block in a Template is {% block head %} (where you can add stuff to <head>).

It also slows the build process a bit, but if you don't do a lot of template->template extends, it's fine.

---

I think having to always use {% block template %} in your Layouts feels like a very small thing to ask in exchange for proper template inheritance, so I'd go for it.

Thoughts?

[lowv-developer]

I cant reproduce your example :/

[cossssmin]

You need the while loop in the toString() method, as I have it above - have you added that locally? I haven't commited anything yet...

[lowv-developer]

yes i did

i added this (i get error with {% extends "${config.layout}" %}:

const layout = config.layout || config.build.layout

and then below

...const nunjucks = await NunjucksEnvironment.init()

if (typeof options.beforeRender === 'function') {
  await options.beforeRender(nunjucks, config)
}

html = `{% extends "${layout}" %}\n${html}`
html = nunjucks.renderString(html, { page: config, env: options.env, css: compiledCSS })

while (fm(html).attributes.layout) {
  const front = fm(html)
  html = `{% extends "${front.attributes.layout}" %}\n{% block template %}${front.body}{% endblock %}`
  html = nunjucks.renderString(html, { page: config, env: options.env, css: compiledCSS })
}

html = ht..

[lowv-developer]

my setup is:

<!-- src/layouts/default.njk -->
// default file

<!-- src/templates/trasactional.njk -->
---
layout: src/layouts/default.njk
---
{% block template %}
{% block button %}Confirm your email{% endblock %}
{% endblock %}

<!-- src/templates/transactional_account.njk -->
---
layout: src/templates/transactional.njk
---

{% block template %}
{% block button %}Confirm your account{% endblock %}
{% endblock %}

[cossssmin]

Added https://github.com/maizzle/framework/commit/8bdb8ddc0f455fb560d33875aa2e3f89183ea987, make sure you're using v0.5.1 and your toString() looks like in the inheritance branch. You shouldn't get any error with config.layout.

Otherwise files look alright, what command are you using?

[j-mori]

Sorry, had to roll it back in v0.5.1 because of several issues:

• maizzle serve was getting an undefined layout, so it was not using any
• all templates were using the same (default) layout, even if they extended a different one

I'm open to suggestions :)

hi! Wasn't the problem caused by the bad layout assignment? just figured out that my advice to use let layout = config.layout && config.build.layout was just bad. do you think something like this will solve the issue? let layout = config.layout != null ? config.layout : config.build.layout

[lowv-developer]

"@maizzle/framework": "^0.5.1",

maizzle serve

i copy/paste the raw file from "Rework template inheritance"

ex. In src/templates/transactional_account.njk I only see the Confirm your account text (no template). Im seeing the all the head stuff in view source

[cossssmin]

hi! Wasn't the problem caused by the bad layout assignment?

Unfortunately no, you could extend as many levels deep as you'd want, so each level will contain a different layout. In order to go up the chain and reach the Layout, while actually rendering block contents, you need to re-render the template, talking the newly-found Front Matter into account.

[j-mori]

Added 8bdb8dd, make sure you're using v0.5.1 and your toString() looks like in the inheritance branch. You shouldn't get any error with config.layout.

Otherwise files look alright, what command are you using?

i get an error Error: template not found: undefined, config.layout is undefined

[lowv-developer]

you need to declare layout in every template!

Added 8bdb8dd, make sure you're using v0.5.1 and your toString() looks like in the inheritance branch. You shouldn't get any error with config.layout. Otherwise files look alright, what command are you using?

i get an error Error: template not found: undefined, config.layout is undefined

[cossssmin]

i get an error Error: template not found: undefined, config.layout is undefined

Just tested with your exact files, it works perfectly fine. Not sure what's causing it for you.

you need to declare layout in every template!

Right, src/layouts/default.njk is no longer automagically set as the Layout being extended, you need to extend it in your Template.

[lowv-developer]

"automagically" is always better! :D

[j-mori]

Right, src/layouts/default.njk is no longer automagically set as the Layout being extended, you need to extend it in your Template.

The problem was this.

I wasn't setting template for every default template via FM and miazzle was crashing, so is this inteded?

[j-mori]

"@maizzle/framework": "^0.5.1",

maizzle serve

i copy/paste the raw file from "Rework template inheritance"

ex. In src/templates/transactional_account.njk I only see the Confirm your account text (no template). Im seeing the all the head stuff in view source

Declaring layout in every template make maizzle compile but i get the same result: transiciotnal.njk work fine but transictional_account.njk show only declared block as plain text + extended template's FM heading as text, layout/default.njk seems not being loaded

[cossssmin]

You're both probably running into a caching issue or something, just tested this and it works with the default layout:

src/templates/first.njk :

---
layout: src/layouts/default.njk
---

{% block template %}
Parent Template
{% block button %}First{% endblock %}
{% endblock %}

src/templates/second.njk :

---
layout: src/templates/first.njk
---

{% block template %}
{% block button %}Second{% endblock %}
{% endblock %}

{{ super() }} added in the template block of second.njk also works.

[cossssmin]

So to get inheritance working properly, the trade-off here is to always have to specify the Layout you're extending from a Template.

I believe I had it work automatically being inspired by the Jigsaw static site generator, which powered the original Maizzle (can't remember if it was a feature, or just something you could just do).

Since we have no better solution, I think we'll go with this - it's a very small trade-off to make for what you get in return.

[j-mori]

Hey cosssmin i pulled a potential fix based on 0.5.0, could u give a try?

With that fix templates will still load default layout from config.js but now its possible to set layout: false inside templates whose doesn't need to automagically include the default conifg.js layout! So now its possible to do something like:

---
layout: false
---
{% extends 'src/templates/transactional.njk' %}
{% block button %}My button{% endblock %}

[cossssmin]

I tried, it, answered there in #74 . Not only does it not compile that template that extends a template, but I don't want users to have to add workarounds like this, with false and {% extends %}.

Thanks for the PR, but I think I'm just going to make specifying a layout: mandatory. It's not a big deal and it doesn't do any magic stuff, which I think is better.

[cossssmin]

There's obviously still an issue with using render() without extending a Layout (simply render a string), working on a solution...

[cossssmin]

Fixed it, will publish soon. @lowv-developer managed to preserve the 'automagic' of layout extending - you should only be required to specify the layout key only if your template is being extended by another template.

[cossssmin]

Please check v0.5.2, it should have fixed the issues.

[j-mori]

Thanks for the fix! just tested out and it seems working pretty fine, but i have another related small problem, maybe i m doing something wrong but seems that the main layout doesn't get compiled.

This is my setup with defaults templates:

src/templates/transactional.njk: added layout via fm and wrapped command inside a block

---
layout: src/layouts/default.njk
---

{% block cta %} Confirm email address{% endblock %}

src/templates/transactional_account.njk:

---
layout: src/templates/transactional.njk
---
{% block cta %} Confirm your account{% endblock %}

The expected result would be two identical html with differents commands text, but in transactional_account.njk some class miss inside <html>,<body> tags, probably layout didn't get compiled as should?

Results: -src/templates/transactional.njk:

<body class="bg-gray-200 h-full">

-src/templates/transactional_account.njk:

<body>

[cossssmin]

Will need to test, but I have a hunch this is purgeCSS or email-comb removing classes. Might have to do with the recursive re-rendering.

This is for the maizzle build command right?

[j-mori]

No i m having the issue with miazzle serve, did'nt tested build command

[lowv-developer]

everything seems to be working fine!

<!-- src/templates/transactional_account.njk -->
---
title: trasactional
layout: src/templates/transactional.njk
bodyClass: bg-gray-200 h-full
htmlClass: bg-gray-200 h-full
---

{% block button %}Confirm your account{% endblock %}

[lowv-developer]

No i m having the issue with miazzle serve, did'nt tested build command

try to delete the folder @maizzle and npm install

when there is inheritance you need to declare layout in every level.

the layout is optional when you have only one level (without inheritance)

[cossssmin]

when there is inheritance you need to declare layout in every level.

This. You need to always declare the layout you're extending, if you use inheritance (extending a template from a template). Just tested and it works as expected, so I'll close this as resolved - feel free to reopen if you spot other issues with it. And big thanks to both of you for your help ✌