Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Twig embed-like functionality could be very useful #790

Open
Onderhond opened this issue Jul 11, 2016 · 64 comments
Open

Twig embed-like functionality could be very useful #790

Onderhond opened this issue Jul 11, 2016 · 64 comments

Comments

@Onderhond
Copy link

Onderhond commented Jul 11, 2016

Twig embed functionality: http://twig.sensiolabs.org/doc/tags/embed.html

Basically it's like an include, but it allows for {% block %} statements. I read through the issues here and found a similar discussion (#539) but that seemed centered around added {% block %} statement to the include function, not so much creating a new "embed" function for it.

I've been looking at various js templating languages the past week and most are seriously underpowered for what I want to do. Nunjucks is so far the only one that looks appropriate, sadly it doesn't support what twig calls "embed". I already looked around for workarounds and even though I found several options, they didn't seem very clean or comfortable to me.

The reason for including a function like "embed" is to make an abstraction of mid-level html structures. Stuff like column layouts, structural lists (the ones with sorting options and pager, not content lists), header-main-footer containers ... anything with a fixed html structure but wildly varying content. I know I could make templates for each instance of these html structures using the extends function, but then I'd have a bazillion templates I'd only be using once.

A practical example maybe: The list embed template

<section class="list {{ class }}">
  <header>
    <h1>...</h1>
  </header>
  <div class="main">
    {% block main %}
        list main content
    {% endblock %}
  <div>
  <footer>
    <div class="paging"> ... </div>
  </footer>
</section>

Using the list embed

{% embed "list" with {class: news} %}
  {% block main %}
    // put all the news items here
  {% endblock %}
{% endembed %}

Ideally I'd like the list embed to extend from a more generic header-main-footer structure template (with three blocks, one for header, one for main, one for footer) and if you're looking to reuse this template over projects you could add some if statement to include/exclude stuff like the pager).

So yeah, hope this made sense and I hope you kinda get what I'd love to do. If anything is still unclear, do ask :)

@carljm
Copy link
Contributor

carljm commented Jul 11, 2016

Yeah, I see the usefulness of that tag. Normally I'd handle those use cases with caller macros, but that only works when you have a single "block" that you want to fill in, not multiple. For multiple, I've passed in content as strings in arguments, but that's not particularly nice.

So I'm not likely to have time to work on implementing this, but if a tested and documented pull request appeared, I'd review and merge it.

@paulmsmith
Copy link

I was just about to put in a request for this. :) I'm having to do exactly @carljm's solution but it's not pretty.

@paulmsmith
Copy link

@carljm Is there a bounty thing for this repo? I'd happily pay towards this enhancement.

@carljm
Copy link
Contributor

carljm commented Jul 11, 2016

No, we don't have a bounty system.

@paulmsmith
Copy link

paulmsmith commented Jul 11, 2016

@carljm Thanks for the swift reply. I'll see if I can drum up some interest from people faster and better than me at Javascript.

@Onderhond
Copy link
Author

@carljm I've been thinking about parsing the content first and passing it along as variables, but with multiple nestings (embeds in block statements of other embeds) it would become quite messy doing it that way. Also somewhat counterintuitive, since you're working inward out (starting with the smallest html components), where html is pretty much always outward in.

The beauty of the embed function would be that it would be in line with the nesting rules of plain old html. Just an extra level of abstraction, which is what a good templating lang should be imo.

@paulmsmith I'd love to help implement this btw, but I'm an oldskool html/css'er with very limited javascript experience (read simple jQuery dom manipulations and fixing everything else in css). If you can't find anyone to implement this I might ask my employer to see if he could help out a little, but no guarantees there.

@howardroark
Copy link

howardroark commented Jul 13, 2016

@Onderhond and @paulmsmith ... How about I make a fork, add you two as contributors and then we kick off a pull request. That way if anyone else passes by and likes the idea they can test and help out. I don't think this will be entirely all that simple :P

Starts here... https://github.com/howardroark/nunjucks/blob/master/src/parser.js#L1272

@Onderhond
Copy link
Author

Onderhond commented Jul 15, 2016

I'm all for it. Afraid I won't be much help on the technical side but definitely willing to bugtest when needed!

@rludgero
Copy link

Hi guys, something new about that?

Is this #794 PR ready for rebase?

i really need this enhancement.

Like @Onderhond, I also use Twig and this functionality is very useful.

Cheers!

@howardroark
Copy link

Hey @Rodrigo-Ludgero ! The fork for #794 is only just a starting place. Basically I duplicated include as embed to map out it's basic structure in the the code and ensure it's following common conventions. It still needs to be adapted to be a morphing of include and extend concepts. I added you as a contributor to the fork in case you felt like giving it a go :P

@rludgero
Copy link

Hi @howardroark , thanks for your prompt response and the invitation, but I'm afraid I will not be of much help to contribute code to the repository.

@robvenn
Copy link

robvenn commented Jul 31, 2016

Hey I'm also interested in this feature, was already adding some code without knowing a branch had been started. I started out in a similar way but assumed that embed tags always require an endembed tag to be valid, added a bunch of tests and now working on the inherited blocks...

@howardroark
Copy link

@robinvenneman Sounds better! Got a link to the code progress?

robvenn added a commit to robvenn/nunjucks that referenced this issue Aug 1, 2016
@rludgero
Copy link

rludgero commented Aug 2, 2016

Hi @robinvenneman , thanks for get to it.

In my opinion, this feature is extremely useful, increasing the ability to reuse more code and add even more power to the system template.

@rludgero
Copy link

rludgero commented Aug 4, 2016

Hi @robinvenneman , perhaps this example https://github.com/twigjs/twig.js/blob/master/src/twig.logic.js#L949 will be useful to create the tag.

@robvenn
Copy link

robvenn commented Aug 10, 2016

Hey @Rodrigo-Ludgero thanks for the suggestion... interesting to see how it's done in twig. However, I currently don't have time to work on this. I checked what's wrong with the code that I added and the embed tag works but the nested blocks are not parsed correctly from the NodeList, perhaps because it has an end tag or because I didn't use the parent template like with the extend tag. Maybe someone else can pick this up?

@howardroark
Copy link

@robinvenneman Yeah... that is kinda where I got lost too. It's certainly not as simple as just copying code from include or extend :P I suspect some refactoring would be in order.

@ArmorDarks
Copy link

Hi guys

As far as I remember, Nunjucks opted to be quite close port of Jinja2.

This means that before rushing out implementation, this issue should be elevated to Jinja2 repository first. There we should lead the talk and decide specs of this feature, ensure that it's properly fitting into Jinja ways.

While I without any doubts seeing usefulness of this addition, I have some doubts in current implementation. embed seems to me like a mix between macro and React component, but in current implementation it seems to be more a synergy of include and something else. It might be more logical to enhance macros instead and allow them to work more like a component, and to use more powerful callbacks instead of a caller(), which would be able to uphold even blocks. This might give us in return extremely wider flexibility.

Anyway, this is something that should be discussed with Jinja2 team first. There we'll be able to get better feedback.

@internalfx
Copy link

I have been working on implementing this for a day now...going into day 2.

@internalfx
Copy link

I'm currently stuck in the compiler....

I have gotten the template loaded, and I have the blocks. But I'm having a hard time getting the block content from the template that called embed and pushing it into the loaded template.

@internalfx
Copy link

it looks like context.addBlock() just pushes the block onto an array.

I suppose that is how the renderer keeps track of which content to use.

digging further....(man this rabbit hole is deep)...

@internalfx
Copy link

ok, here is my current parseEmbed implementation....

    parseEmbed: function() {
        var tag = this.peekToken();
        if(!this.skipSymbol('embed')) {
            this.fail('parseEmbed: expected embed', tag.lineno, tag.colno);
        }

        var args = this.parseSignature(true, true).children;
        this.advanceAfterBlockEnd(tag.value);

        var node = new nodes.Embed(tag.lineno, tag.colno);
        node.template = args[0]
        node.contextVar = null

        if (args.length === 2) {
          node.contextVar = args[1]
        }

        node.body = this.parseUntilBlocks('endembed');
        this.advanceAfterBlockEnd();

        return node;
    },

node.body now contains the declared blocks to inject into the embedded template.

@internalfx
Copy link

I recently discovered that nodes have a findAll function.

In my compile method I am able to get the blocks in node.body with this...

node.body.findAll(nodes.Block)

getting closer....

If there are any maintainers here, feel free to chime in with pointers.

@internalfx
Copy link

😵

I feel like I'm gonna have to modify the render method itself.

I simply can't seem to find a way to get the parents template blocks to override a childs.

@paulmsmith
Copy link

@internalfx Loving the updates. 👍

@internalfx
Copy link

Finally got it working....

first working version

internalfx@e8f6ab1

@rludgero
Copy link

@internalfx great job 💻 💾 , thank you very much for having begun to create the tag. 👍 👏

@internalfx
Copy link

My vote is that if compatibility can be maintained without sacrificing features, then it's fine to keep compatibility.

If it gets in the way of innovation, it's gotta go.

My 2 cents. (I am a bit of a "move fast and break things kinda guy")

@rludgero
Copy link

It wasn't about compatibility with Twig. It was about how cool embed was.

Embed saves a lot code.

This functionality is what stands out above all others templates engines I've used.

And yes, embed is very cool!

@internalfx, once again, thank you for using your time in creating this tag.

@carljm
Copy link
Contributor

carljm commented Aug 25, 2016

@internalfx Yes, sharing templates between two platforms is a real use case, and one I'm absolutely not going to sacrifice. For me personally, the only reason I ever picked nunjucks in the first place was because it allowed sharing the same templates between Python server and JS client; every project I've used nunjucks on did this. It's hugely valuable to be able to do that.

@internalfx
Copy link

To everyone who has appreciated the work on the embed tag, you are quite welcome, enjoy.

@carljm We clearly have different priorities in development, but in this case it doesn't matter.

embed does not break any compatibility that I can see, it only enhances it's compatibility with Twig.

Everyone gets what they want. (except @ArmorDarks I guess)

@howardroark
Copy link

howardroark commented Aug 26, 2016

If we get jinga to accept a PR, would we be able to merge this?

@Onderhond
Copy link
Author

@internalfx the compatibility is a bonus for us. It's important, but the actual embed functionality is the part that is crucial of course. I know there's already some divergence between nunjucks/twig/jinja2, but so far the changes are quite minimal and most importantly, manageable.

In the past we used a custom templating language (xml-based) exactly because templating languages 7-8 years ago typically didn't have the embed functionality (mind you, this was for generating static templates, not live code). If you want to maximize reuse of code, it's an essential tool imo. So again, kudos for putting in the hard work!

@carljm
Copy link
Contributor

carljm commented Aug 26, 2016

I already said above that this feature doesn't break compatibility with jinja2, and I'm not going to block it on a matching feature in jinja2. It's just waiting on me (or one of the other maintainers) having time to review.

@ArmorDarks I'm open to more discussion, but for that to be useful I think you'll have to be able to put your concerns into more concrete form, with a proposed alternative.

@carljm
Copy link
Contributor

carljm commented Aug 26, 2016

@ArmorDarks you're also free to open an issue on jinja and get their feedback on the idea. You can point to the fact that Twig (based on jinja2) decided to add it.

@ArmorDarks
Copy link

That would be quite weird to make an issue to Jinja2 repository by person, which has been quite doubtful in this feature consistency, I think.

I don't have any meaningful proposals for now, and I don't want to be a douchebag which only criticizes and blocks all other people work. In fact, I never intended to. I only had some worries regarding possible collisions with Jinja2 maintainers view. In all others senses, I'm not opposed to embed functionality and see it as welcome addition to Nunjucks.

@allmarkedup
Copy link

Really looking forward to seeing this getting merged in and released 👍 Twig-style embeds are a massive help when developing templates for component/pattern libraries and this would really make Nunjucks a much more useful tool for this purpose. Great work @internalfx.

@allmarkedup
Copy link

@internalfx Just a heads up - I'm just testing this out using your fork and it seems to be throwing an error when I try and specify context data using with. This works:

{% embed "panel" %}
    {% block content %}
    Panel content
    {% endblock %}
{% endembed %}

This throws an error for me (parseSignature: expected comma after expression):

{% embed "panel" with { foo: 'bar' } %}
    {% block content %}
    Panel content
    {% endblock %}
{% endembed %}

I haven't dug into it to try and track down the problem but it's probably worth seeing if you can replicate - assuming with support is part of your PR?

@internalfx
Copy link

@allmarkedup context data not supported.

nunjucks doesn't seem to have lexing for arguments like that.

@allmarkedup
Copy link

@internalfx Ah ok sure. Hence why it's not supported for the include tag either, makes sense.

It would be a great addition in the future though, both for include and embed tags (although I understand that people feel the 'nunjucks way' is to use a macro instead). I'd attempt to dig in myself but the lexer/parser logic is a bit above my pay grade I think ;-)

In that case, it's all working great in my testing so far 👍

@robvenn
Copy link

robvenn commented Aug 29, 2016

@internalfx perhaps some unit tests could be useful to verify the code? I had added some on the branch that I forked.

I've tested with your code and the only thing that doesn't seem to work is the "ignore missing" feature, which might not be needed for this tag anyway. Other than that I've just tried out some basic stuff and it seems to work fine.

@internalfx
Copy link

Thanks @robinvenneman

I have invited you as a collaborator to my fork.

You can push your tests directly to the fork, and they should land in the PR.

@internalfx
Copy link

@robinvenneman I'm not certain about the "ignore missing" either...I just removed it.

Seemed unnecessary.

@robvenn
Copy link

robvenn commented Aug 30, 2016

@internalfx alright, pushed the tests for parse & compile, except for the "ignore missing" test. Also noticed the comment on the pull request about the tests so I hope this is enough to get it accepted.

@romannurik
Copy link

In case anyone is looking for an alternative that works with imports and macros (rather than includes), I've put together a quick Nunjucks extension that can do that. Super useful for something like Angular's multi-slot content projection (or AngularJS's named transclusion slots).

Really quick example usage:

{% blockcall myLayout(title='Can pass in args as normal') %}
{% argblock side %}
    This is an example arg <em>sent as a block</em>.
{% argblock content %}
    <p>And this is another keyword arg</p>
{% endblockcall %}

This would call a macro previously defined like so:

{% macro myLayout(title, side, content) %}
...

@ArmorDarks
Copy link

Do be honest, I do not understand why you would do so. There are already macros for same purpose.

@romannurik
Copy link

@ArmorDarks can you share an example? I was previously using call and caller() but needed multiple-caller support which didn't seem to be there.

@ArmorDarks
Copy link

ArmorDarks commented Nov 13, 2017

In most cases you can use regular macro and instead of call and caller pass in set blocks:

{% macro myLayout(title, side, content) %} ... {% endmacro %}

{% set title %}
<h1>title</h2>
{% endset %}

{% set side %}
<aside>Side content</aside>
{% endset %}

{% set content %}
<div>Main content</div>
{% endset %}

{{ myLayout(title, side, content) }}

However if you end up passing something like this into macro, it is good indication that most likely you're doing something wrong and trying to impose macro as a layout. For those purposes regular blocks alongside with extending should be used.

Inability of macro to contain few calls indeed a limitation and I wish it didn't exist. But to solve it, we should take issue to the Jinja repository first. And, in general, it nor related to this issue, nor an additional extension is a proper solution.

@romannurik
Copy link

@ArmorDarks I thought about doing it this way, but it felt a bit too cumbersome. At any rate, embed, especially if it supported with, would definitely be a better solution.

@ArmorDarks
Copy link

As I've stated previously, I have a strong feeling that we should validate ideas with Jinja community first. Than we will get better feedback.

From my point of view, embed slightly violates Jinja original approach and introduces even more ambiguity between few already similar concepts: macros, includes and extends. As far as I remember, original creator of Jinja stated that he wished never to introduce include at all, because it brought that ambiguity. embed continues that path.

gonogo pushed a commit to gonogo/nunjucks that referenced this issue Nov 30, 2017
@arechsteiner
Copy link

arechsteiner commented Nov 27, 2018

As far as I remember, original creator of Jinja stated that he wished never to introduce include at all, because it brought that ambiguity.

This is like saying "we're not gonna do screws, we already have nails and we don't want ambiguity".

I've started missing embed very quickly after adopting nunjucks and neither include nor extend solve the problem very well. It's needed simply because it is very useful in certain situations.

@danburzo
Copy link

danburzo commented Aug 2, 2019

Hi everyone, until we have an official embed tag, I tried my hand at implementing a custom one: nunjucks-embed. However, I had little success in grokking how custom tags should be implemented, and I was hoping someone could provide assistance with these two... limitations 😅:

  1. Works as {% embed with context "template.njk" %} instead of {% embed "twemplate.njk" with context %} because... that's the only way I could make it work; but ideally it should match the include signature
  2. I can't figure out how to pass the with_ctx flag to my run() method...
  3. Sadly in a for-loop it can't pick up the current item 😕

Source is here:
https://github.com/marceljs/nunjucks-embed/blob/master/index.js

(H/T to @allmarkedup's nunjucks-with for giving me a starting point!)

@ncjones
Copy link

ncjones commented Aug 22, 2021

Wow 5 years. What's the hold up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests