-
Notifications
You must be signed in to change notification settings - Fork 71
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
Dynamic Partials and Partial Collections through Coercion #54
Comments
The proposed syntax:
and
Assume there is a key named 'partial' and then looks up that partial. The idea of dynamic partials is great, but I don't know about assuming a certain key to be present is the most developer friendly approach and the easiest to understand quickly. It could instead by {{>.partialName}} which, when the lookup for the literal key '.partialName' fails, it resolves .partialName to 'text' and looks that up. It's clearly valuable, but I don't think it fits in the spec for 2.0 |
See my response here: janl/mustache.js#242 (comment) As for it not fitting the 2.0 spec, there is currently no way to render partials dynamically which I see as a significant drawback. This is fine for smaller projects as you just add a boolean to each object in a collection such as View {
items: [
{ type: 'image', url: 'Some URL', is_image: true },
{ type: 'text', content: 'Some text', is_text: true }
]
}
But... as soon as this goes above a couple of types, things get hard to manage. The whole premise of Mustache is to keep logic out of the template and this reads to me as a bunch of Wouldn't it be nicer to be able to do:
So much cleaner, don't you think? Sorry to labour my point, but I'm using this in production and it's changed my life :) |
@thelucid As usual, when one's code contains too many
And, if you absolutely want to use partials:
And voilà, no need for a spec update! |
@groue Ew, I'm sorry but this is exactly why the functionality is needed. Returning chunks of html from the view is in no way a good solution to the problem... I would hope I'm not alone on this. This is in fact a perfect illustration as to why we need a clean solution to this problem, if hacks like @groue is suggesting are being suggested then something is fundamentally wrong. |
@thelucid You call it a "hack" because you see Mustache spec issues history is full on this kind of situations, if you read them carefully. So, in a way, I gave an answer that is typical of the "Mustache 1" spirit. Be free to suggest improvements for Mustache 2. But dont' call "hacks" ways to use Mustache that you just don't happen to like. |
@groue I'm sorry, I just don't see that returning a chunk of html from a view object (be it a view model or a data model) is a maintainable solution. Where do you draw the line, you may as well bypass Mustache completely and just interpolate strings. I would much rather do:
|
@thelucid Have you read my message? Check after the "And, if you absolutely want to use partials:" sentence. |
@groue Yes, I have read the message and there are a few problems with the suggested partial solution:
Injection example: {
items: [
{ url: 'Some URL', html: function() { return '{{>image}}'; } },
{ content: 'Some text', html: function() { return '{{>text}}'; } },
{ html: '<script>alert("I am hijacking your page")</script>' }
]
} |
As much as the
Single interface + Liskov substitution principle is no DRY for you? Gosh, I can't see how more DRY it could be.
Your solution must keep synced the type value and the partial name, which is just the same chore.
Is it? open a new issue, and ask for a removal of the triple mustache. Listen: I won't defend my proposal more than necessary. Actually, I'm quite sure you didn't know about the lambda before I introduced them to you. Think more about the tool, make sure you know every tiny details: Mustache is more rich that it looks. Maybe you'll find a solution that conforms to your standards with the spec as it is. |
I'm at a loss as to why you're against a clean solution to dynamic partials.
That is exactly why I am suggesting such a feature, it is dry and concise.
You are still duplicating a function which is not DRY.
Yes, but you are just specifying the partial to render, not a function that actually renders a chunk of html.
The triple mustache doesn't escape the value by design, that's what it's for and how it works. So yes, your example is open to injection attacks... try it if you don't believe me: View {
html: '<script>alert("See, I have control");</script>'
} Template
I did know about lambdas before you mentioned them (that's a little patronising, especially given your lack of understanding when it comes to the triple mustache), I just don't see them as a good fit for this particular problem. We'll just have to agree to disagree on this one.
There is currently no way to render dynamic partials other than the suggestions we have already exhausted here, hence this ticket. |
I'm not against your solution. I just wanted to make sure everybody knows that there are already existing solutions. Usually, people ask for a feature because they don't know their tool enough. Sorry if I have patronized. About the triple mustache: they allow injection in all situations, not only in the one we are talking about here. What I meant was that the topic of injection should hence not pollute this topic, and that you can ask for the removal of the triple mustache in some other issue. |
I think @groue has a great point. This is already possible, therefore it's best not to add a feature for it. I vote no. |
It's possible to drive a nail into a block of wood with a mallet but it's not necessarily the best tool for the job. |
I think letting And with @bobthecow's suggestion of moar dot notation hotness, you could do |
Yes, that's not too bad, what about the {{@collection}} shorthand? |
The only potential problem with that solution is that it's a little ambiguous i.e. the same syntax does two different things. Having a separate syntax makes it easier to see what is going on. |
That should be explicit, if that's what you want. Otherwise you're opening yourself up for exploits. Maybe use a "variable dereferencing" type token? |
What about using the
or shorthand that assumes a
I would personally prefer it being implicit, going with a convention whereby a
and shorthand:
The Has anyone tried a few examples using my patch to |
Not a fan of using @bobthecow Could you give an example of being exploited? We could also use another interesting syntax:
It seems the only way we dodge ambiguity is by having a special char or marker that means lookup the value of this string not the name 'string' |
In response to the dereferenceing thing, |
|
Essentially we're talking about the same thing - some symbol to 'dereference' the variable. I really like @bobthecow's |
Not keen on the taches in taches thing, wouldn't that choke the parser? I'm currently leaning toward:
with a shortcut of:
or
|
The I wonder how a lambda/filter/helper could assist you with a shorthand version... What does everyone think of having a deference operator, like
|
I'm not against using With regards the shorthand version, in practice this is more useful than the longhand. In fact, I'm using this setup in a couple of projects and have not actually had a need for the longhand version over the shorthand. The resulting templates are super clean and concice. Let's say you have a page with main content and sub content, and each contains a collection of objects, this becomes (using
I would be more interested in the shorthand making it in over the longhand, be it |
@thelucid : are you proposing to reserve a special key |
Not to necessarily reserve it but the convention would be that when rendering a collection of partials, use the I do quite like the |
@thelucid : Don't play on words: you are indeed "blessing" the Of course, you are feeling it is a bad idea. And you are right: it's a big threat on compatibility. Each time a special key is added to the spec, one has to bump the Mustache version by a major increment (Mustache 2.0, 3.0, etc.) since it could break existng code. You can give up this idea right away - it will never enter the spec. |
I'm not playing on words, this functionality is backwards compatible, if you took the time to think about it or try out the patch before mouthing off you would see that. I'm just throwing some ideas out there that I have found to be hugely beneficial in my own projects and that I thought others could benefit from. This ticket has thrown up some interesting discussion around the Your negativity is getting on my nerves and don't tell me what or what not to do. Go find another ticket to troll. |
It was interesting reading this. Almost two years later, heh. |
bump Any movement on a v2 spec that includes this feature? |
^ |
To anyone still hoping for a feature that involves detecting a hard-coded name such as To anyone still looking for a way to dynamically select a partial based on context, I suggest the following approach based on feature detection. Data: {
items: [
{ content: 'Some text' },
{ url: 'Some URL' }
]
} Templates: {{!base.mustache}}
{{#items}}
<p>{{>polymorphic}}</p>
{{^items}}
{{!polymorphic.mustache}}
{{#content}}
{{>text}}
{{/content}}
{{#url}}
{{>image}}
{{/url}}
{{!text.mustache}}
{{content}}
{{!image.mustache}}
<img src="{{url}}"/> I know, I know. This looks suspiciously like the code that @thelucid was trying to avoid. The
Last but not least: this already works, even without the lamdas extension, so you don't need to wait for anyone to change a specification in highly controversial ways. |
In my template I want to dynamically load contents of a page, following your example this is what my code would look like:
I could use lambdas to solve this but I find it kinda tedious (not to mention the fact that this way I'm still tagging the data). <?php
$content_file = "foobar.mustache";
ob_start();
include("template.mustache.php");
$content = ob_get_clean();
// (mustache rendering here)
?> template.mustache.php: {{>header}}
{{><?=$content_file;?>}}
{{>footer}} Pretty bad right?
We don't want to change a specification, we just want to add a feature, a new token.
Templates:
Pretty simple, right? |
I'm new to this discussion and the top post has mostly dead links. Can proponents of the change point to a post that clearly explains the feature being proposed? I have the impression that some use-cases are in common with "template inheritance" which was finally accepted as an optionally-supported feature (see #125): base.mustache:
main.mustache:
aboutpage.mustache:
(This can be extended to pass a title to the base template, etc.) |
Yes, you're right, some use cases are common. Data:
Templates:
|
@gasche The originally proposed feature is described in detail in janl/mustache.js#242. @anomal00us You are proposing a different feature, although it serves the same purpose and the suggested syntax may have been inspired on the discussion. To be honest, I find your proposal more sensible than the original. The syntax is less surprising and it doesn't involve a hard-coded special name. I must point out that this is the spec repository. If you only want to add a feature to one particular implementation, you should probably take the discussion to that implementation. There is nothing wrong with discussing it here (again, as I said, I actually like what you're describing), but anything discussed here is about changing the specification, by definition. |
Following up on what I just wrote: I realized that the syntax I ended up deciding that I like the language more in the minimalist form that it has today. As far as I'm concerned, there is still room for dynamic partials with a |
Yes, I know. I already added this into my implementation, that's also why I am here.
Can you make an example?
Template:
I totally agree.
So if we're writing a pull request for this we're going to keep things simple and allow deferencing only on simple data? |
Ah I think I got it. Making * a deference operator and allowing to combine it with all the other tags, right?
Well, I don't feel like this is a good idea.. it's probably better to keep things simple. So yeah, my proposal is to have |
@anomal00us
Honestly, I am probably not going to write a pull request for this, at least not soon, because I haven't implemented the feature myself (yet). But if you were to write one, I would definitely be in favor of keeping it simple, allowing only single-level dereferencing specifically for the purpose of interpolating plain partials (i.e., not parents). Basically, |
Great, I'm planning to write it soon. @jgonggrijp do you know if there is a tool for getting the json from the yml, or did people here write everything by hand? |
Okay I made PR #134 |
@anomal00us There is a command for it, which is mentioned in the Readme if I'm right. I suppose you already found it. |
Probably a bad idea, but what if we spec general purpose dereferencing instead? {{ *variable }}
{{# *list }}
{{> *template }}
{{/ *list }} {
variable: "a",
list: "b",
template: "c",
a: "foo",
b: [
{name: "bar"},
{name: "baz"},
],
c: "somepartial"
} {{! somepartial.mustache }}
{{ name }} Results in…
|
It crossed other people's minds before, so at least it's not stupid. It was first discussed in 2012, from #54 (comment) onwards. That never led to anything (at least not in the spec). It was again discussed by me and @anomal00us just a few days ago, from #54 (comment) onwards. Bottom line of the latter discussion is that we both preferred not complicating the tag contents. See also #134. For me personally, Mustache's syntactic simplicity is one of its main selling points. A general purpose dereference operator would arguably make the language even more powerful than it already is, but I think the added value is questionable for all tags except partials (where it enables a template equivalent of polymorphism through tagged unions, similar to how parents and blocks enable polymorphism through inheritance). I feel it would also start a slippery slope towards a bloated language like Handlebars or Jinja. Besides dereferencing, can we apply other general purpose operators within tag contents? Can we dereference multiple levels? Etcetera... it's a direction I rather not explore. On another note, I would be highly interested in your opinion on #131. |
ahahahahah i clicked through from #134 and didn't expand all the collapsed past conversation here and missed that I proposed this exact thing a decade ago when it first came up 😛 |
The question of dynamic tag names has come up for more than just partials in the past. To me it feels far more consistent to add a general purpose dereference operator (while I do acknowledge the slope we start slipping down) than to introduce a special form of partial that pulls the template name from the view model. |
My question is this: can't we make a hard choice to not start slipping down that slope? We could have dereferencing in other ways. For example, we could give lambdas (read-only) access to the context stack through an additional argument. |
Adding dynamic partials, discussed in #54
Addressing a comment by @bobthecow in #134 (review) ref #54, #134
I agree with @bobthecow a general purpose dereference operator might be useful when trying to reference things that would normal iterate or be conditional especially in the context of #135 . That is the only need of dereferencing in my implementation since we don't support dynamic partials (or templates in general) is to pass lists and booleans around to lambdas. Currently the easiest solution for my case is to use virtual keys instead of a new sigil. e.g. to reference a list instead of iterating I do the following: {{#someList.this}}
{{#someLambdaExpectsListOnTopOfStack}}
{{/someLambdaExpectsListOnTopOfStack}}
{{/someList.this}}
I am starting to get the idea that a large amount of problems that should be easy in mustache can be solved with virtual keys notable index information (e.g. Virtual keys are much easier to add to existing implementations than a new sigil albeit I'm not sure if that holds up to dynamic partial needs (to be frank I'm like @gasche in that I'm not sure if I even follow all the ideas in this thread). Otherwise I think this can quickly become what Handlebars is which I don't think we should go down that path. |
@agentgt In my particular implementation, adding virtual keys like Power lambdas would make it possible to add your own virtual keys, but we already discussed that in #135. I understand it doesn't help in your case, and I figure that your implementation probably does already inspect key names inside the template, since it aims to typecheck everything. Mustache was clearly conceived as a dynamically typed language. For context, dynamic partials did make it into the spec eventually, in #134. |
I've just created a pull request over at janl/mustache.js#242
I am using this solution in a couple of projects and it addresses a number of concerns I often hear with regards dynamic partials in Mustache, please see ticket (janl/mustache.js#242) and documentation here:
Really, I urge everyone to have a play with it as it has cleaned up my templates no end.
Kind regards,
Jamie
The text was updated successfully, but these errors were encountered: