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

Implement Helpers. #139

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement Helpers. #139

wants to merge 3 commits into from

Conversation

janl
Copy link
Owner

@janl janl commented Nov 9, 2011

See examples/helpers.js / .html for usage.

I'd appreciate a review :)

cc @lapidus @bcherry @bobthecow @khamidou

See examples/helpers.js / .html for usage.
@funkatron
Copy link

+1 (I have nothing intelligent to add)

@benoitc
Copy link

benoitc commented Nov 9, 2011

looks useful. Maybe it can have a differet syntax though like:

{{name|ucase}}

So you could eventually pipe results.

@MartinodF
Copy link

I'd love to have this, but it seems to violate mustache's "no logic in templates" principle. Helpers are explicitly called out in the mustache readme ("Instead of views consisting of ERB or HAML with random helpers...").

Still, I think simple helpers would really help streamline some of my views' logic.
+1!

EDIT: also, have you seen handlebars.js?

@pegli
Copy link

pegli commented Nov 9, 2011

I agree with Benoît -- it would be nice to have a better-defined syntax. How about something like this?

{{~helper varname}}

That would fit conceptually with blocks {{#varname}} and partials {{>partialname}}

@janl
Copy link
Owner Author

janl commented Nov 9, 2011

@benoitc not a bad idea at all. See my latest commit :)

@pegli I don't like to introduce more magic symbols, so I went with @benoitc's suggestion.

@MartinodF the idea is directly stolen from handlebars :) c.f. http://writing.jan.io/mustache-2.0.html

@lapidus
Copy link

lapidus commented Nov 9, 2011

+1 great job

This is one use case of helpers I had in mind when looking for a i18n helper implementation:

listOfAvailablePlaneTickets = [ {id : "se", quantity : 5}, {id : "fi", quantity : 1} ]

Should render something like this:

<ul>
    <li>Sweden - 5 plane tickets left</li>
    <li>Finland - 1 plane ticket left</li>
</ul>

With a template such as:

<ul>
    <li>{{id | i18n}} - {{tickets_left | i18n}}</li>
</ul>

But not having much experience with i18n (nor Mustache), I'm not sure this is a sound way of doing it?
(Neither am I sure how the "quantity variable" (that determines singular/plural) in this example should be handled when passed to the i18n helper?)

mustache.js Outdated
var helper_name = name.split("|");
if(helper_name.length > 1) { // we have at least one helper
name = helper_name[0];
helpers = helper_name.slice(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be cleaner as something more like this, although I'm not sure if it would be faster or slower, which would be worthwhile to know:

var helpers = name.split("|");
name = helpers.shift();

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with your suggestion as it wins in clarity. I had to change the if() condition before iterating over helpers to account for the fact that if([]) is truthy.

I didn't benchmark this, but I assume that native functions will be faster than my original hackery. I wouldn't mind a jsperf shootout though.

@bcherry
Copy link
Contributor

bcherry commented Nov 13, 2011

You asked for a code review, so I did one :)

I generally really like this addition. Great stuff!

@janl
Copy link
Owner Author

janl commented Nov 13, 2011

@bcherry awesome review, thanks :)

@mjackson
Copy link
Contributor

-1 I'd rather not add this patch. It would be much cleaner to keep all logic in your view code, where you can document it properly and properly handle any errors that occur.

@janl
Copy link
Owner Author

janl commented Dec 19, 2011

@mjijackson a -1 is a bit strong here for a feature you don't have to use. Why block a feature that others find useful but that doesn’t interfere with your work?

@mjackson
Copy link
Contributor

@janl Agreed, my -1 was a bit too strong. I guess the real question I have is how much do we care about the size and feature set of the code base? I'm ok with adding stuff that's not in the spec, in the hopes that it will find its way into the spec someday. However, I don't see a lot of value in adding features that unnecessarily add complexity to templates.

It's been my experience when talking with others about mustache that they don't fully understand the concept of a view. They think about it as a simple hash of variables, similar to what you get in jsp-style (or erb) templates. For me, one of the killer features of mustache is the ability to reference a function in the template that lets you hide the logic in your view. Education about this feature would go a long way towards helping users understand they don't really need handlebars-style filters at all.

@janl
Copy link
Owner Author

janl commented Dec 20, 2011

@mjijackson my line of thinking went exactly along the path you outline and I came to the conclusion that helpers would indeed be a good nonstandard addition.

@erikschoel
Copy link

I actually implemented pipe based helpers in a similar way janl did.

What I had to achieve was the replacement of CRLF with
in order to render plain text properly. I found there was no way to do this through a function due to the "that.escape" call within render_tags, that was applied after my function substituted with
tags, which would also escape my
tags. And because I didn't want a complete unescaped value back I didn't want to use {{{ either.

What I effectively tried to achieve was "replace CRLF with
on the final (default) escaped result" and the only way I found that possible was to hack in a helper for this.

I started in the "find" function like janl did but then moved the logic to the "render_tags" function as a "post processing" helper for the reason that I wanted to work with the final (default) escaped result.

I now see that the 0.5.0 code has changed significantly, so I would really be very very pleased if the helper functionality would become an integral part of mustache.

Maybe, as a question to both mjijackson and janl: How would you guys suggest I achieve the result I desire?

Thanks and regards,

Erik

@janl
Copy link
Owner Author

janl commented Feb 23, 2012

I think we can start an extras branch and implement these things there. What do you think?

@erikschoel
Copy link

Yes, let's do that! Which version you think we should fork? (as the latest seems to have been a considerable refactor)

@janl
Copy link
Owner Author

janl commented Feb 24, 2012

yeah, base your work off master, awesome! :)

@tonyouyang
Copy link

When will this feature be merged into master , or 0.4.x branch ? I really need it.

@tonyouyang
Copy link

Hi, I'd like to split text into several parts and then render them respectively. I have no idea about writing the "close" tag. Shall I code like:
{{#text|split}}<p>{{.}}</p>{{/text}} ?

@mjackson
Copy link
Contributor

I'm taking another look at this work (sorry it took so long!). As far as I can see you are testing with the following template:

Hello {{name}}.
Hello {{ucase name}}.

Presumably, the code to render this template could look something like this:

Mustache.render(template, {
  name: 'michael'
}, {
  ucase: function (string) {
    return string.toUpperCase();
  }
});

In the above snippet the Mustache.render function takes three arguments: the template, the view, and a helpers object. In my templates, whenever I've needed to do something similar I've always done something like this:

Mustache.render(template, {
  name: 'Michael',
  ucaseName: function () {
    return this.name.toUpperCase();
  }
});

There are a few things I like about this style over the helpers style:

  1. The Mustache syntax is simpler. Just variables, no spaces.
  2. The this reference in the ucaseName function gives you great locality of reference. You just need to look in the same object to find the property being referenced.

I think the added complexity of adding helpers outweighs the gains in this case. Of course, there could be much more complex examples where we might see some significant gains, but I've never seen any in practice.

I also think that the locality of reference helps devs to make better use of their view objects and makes the code easier to read for other devs later on.

@manxomfoe
Copy link

I'm gonna side with the first example. Anecdotally, I ran into this case
the first time I picked up this library, about 2hrs in. So it does happen
in practice. Maybe not much...

What was frustrating was that I wasn't able to make a generic helper. Every
time I needed to reformat a date in my template, I had a new function to
write. It was blatantly verbose. I can't imagine someone overcoming the
basic usage of this library, but still find passing an argument into a
function too complex.

Not tring to be argumentative, just add a data point for you. I appreciate
your work, and enjoyed learning Mustache.js
Thanks.

On Friday, August 31, 2012, Michael Jackson wrote:

I'm taking another look at this work (sorry it took so long!). As far as I
can see you are testing with the following template:

Hello {{name}}.
Hello {{ucase name}}.

Presumably, the code to render this template could look something like
this:

Mustache.render(template, {
name: 'michael'}, {
ucase: function (string) {
return string.toUpperCase();
}});

In the above snippet the Mustache.render function takes three arguments:
the template, the view, and a helpers object. In my templates, whenever
I've needed to do something similar I've always done something like this:

Mustache.render(template, {
name: 'Michael',
ucaseName: function () {
return this.name.toUpperCase();
}});

There are a few things I like about this style over the helpers style:

  1. The Mustache syntax is simpler. Just variables, no spaces.
  2. The this reference in the ucaseName function gives you great
    locality of reference. You just need to look in the same object to find the
    property being referenced.

I think the added complexity of adding helpers outweighs the gains in this
case. Of course, there could be much more complex examples where we might
see some significant gains, but I've never seen any in practice.

I also think that the locality of reference helps devs to make better use
of their view objects and makes the code easier to read for other devs
later on.


Reply to this email directly or view it on GitHubhttps://github.com//pull/139#issuecomment-8207306.

Brian Duchek
++ http://about.me/brian.duchek
++ "There are two ways to write error-free code. Only the third one works."
+++++++++++++++++++++++

@techhead
Copy link

In response to @manxomfoe and @mjijackson above, couldn't something similar be accomplished using a syntax like.

Hello {{#name}}{{ucase}}{{/name}}

And

Mustache.render(template, {
  name: 'Michael',
  ucase: function () {
    return this.toUpperCase();
  }
});

Here the function is working on the context at the top of the stack. I haven't looked at the code in a while, but I presume that the lambda is not currently being called in the context (this) of the top of stack but rather in the one in which it was defined?

I understand that this change would break existing method calls then. However, if that is the case, shouldn't the following work?

Hello {{name.toUpperCase}}
or
Hello {{#name}}{{toUpperCase}}{{/name}}

I'm really looking for a good solution to the singular/plural problem that @lapidus described.

@techhead
Copy link

@janl I've been thinking about this some more. I like the syntax {{tag|helper|helper2}}. But I'm not sure that it's necessary for there to be a different class of helpers. These can all be plain old lambdas.

{{name|ucase}}

could be shorthand for

{{#name}}{{ucase}}{{/name}}

So where the following are functionally equivalent in many cases:

{{a.b.c}}
{{#a}}{{#b}}{{c}}{{/b}}{{/a}}

These would be EXACTLY equivalent.

{{#a}}{{#b}}{{c}}{{/b}}{{/a}}
{{a|b|c}}

And the code for the first example could look something like this.

Mustache.render(template, {
  name: 'Michael',
  ucase: function () {
    return this.toUpperCase();
  }
});

So, first 'name' is pushed to the top of the context stack and then 'ucase' is called against the top of the context stack. Whatever it returns as a value would also be pushed to the top of the context stack for any next lambda to operate on if there was another name separated by a pipe on the tag. So these "helpers" can be chained as previously mentioned.

@manxomfoe
Copy link

I like where this is going, and think that the piped helpers notation would make sense for me.

@bobthecow
Copy link

There's a tl;dr on what will most likely be Mustache.php's implementation of this here. Particularly relevant bits:

The assumptions here are that:

  1. Pipe notation is analogous to dot notation — it can be thought of as syntactic sugar for nested sections.
  2. Filters are just variables in the normal context stack.
  3. Values are not coerced into strings (or escaped) until they come out the other end of the pipe.
  4. Unlike nested sections, the first value in the pipe is fetched prior to passing it to the next lambda.
  5. If any filter is not found, or is non-callable, an UnexpectedValueException is thrown.

Also:

  • Filters don't get parameters. That's logic in your template. See the commentary in the spec issue regarding {{ foo | date.iso8601 }} vs {{ foo | date: "%j. %F %Y" }} (or whatever).
  • Since this is a non-spec feature, it is toggled with a {{%FILTERS}} pragma tag.

@techhead techhead mentioned this pull request Oct 1, 2012
@techhead
Copy link

techhead commented Oct 2, 2012

@bobthecow I've added an implementation here: #261

I've taken a slightly different approach for your numbers 4 and 5 above. 1-3 remain the same.

If a named value found in the context stack is a list, that whole list becomes the context of the lambda. This allows for reducing functions.

{{list|helper}}

does not do the same thing as the two below

{{#list}}{{helper}}{{/list}}
{{#list}}{{. | helper}}{{/list}} (same as line above)

My naive implementation does not complain if a "helper" is not found.

{{a | notfound}} will just render empty regardless of the value of 'a'
{{a | notfound | found}} will call 'found' with 'undefined' as the value on which it is acting
{{notfound | found | a}} will just show the value of 'a'

@techhead
Copy link

Ok, all. Not sure anyone is still following this discussion, but I thought that I'd add one last thing. After my POC fork of Mustache.js, I decided to go after Hogan.js and I took a slightly different approach with it.

There's some documentation on my pull request here: twitter/hogan.js#113

And I'll likely be maintaining a separate fork here: https://github.com/techhead/hogan.js

I'll try to get docs up on that ASAP.

I'll probably leave my Mustache.js fork up for another week or so and then delete it. My future efforts will likely revolve around Hogan.js.

@groue
Copy link

groue commented Nov 2, 2012

(Cross-post from #261)

There have been a debate at mustache/spec, but it looks dead now: mustache/spec#41

GRMustache, the Objective-C implementation, has chosen the parenthesis syntax, {{ f(x) }}, for "filters" : https://github.com/groue/GRMustache/blob/master/Guides/filters.md. Highlights are the possibility to build other expressions on top of filtered values, like {{ last(people).name }}, and filters that take several arguments, as in {{ localize(date, format) }}.

I'd be happy that janl/mustache.js considers this option, regardless of the trend for pipes today. You can read arguments against pipes at https://github.com/groue/GRMustache/blob/master/Articles/WhyMustacheFilters.md.

Since very few Mustache implementations have shipped today with piped helpers, I think it's still time to prevent this bad idea to spread. All, what do you think?

@groue
Copy link

groue commented Nov 2, 2012

I forgot another highlight of GRMustache's filters: their ability to behave like Handlebars' helpers. Compare this "each_with_index" handlebars gist https://gist.github.com/1048968 to this GRMustache sample code: https://github.com/groue/GRMustache/blob/master/Guides/sample_code/indexes.md

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

Successfully merging this pull request may close these issues.

None yet