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

Spacebars nested exprs #4101

Merged
merged 6 commits into from Apr 23, 2015

Conversation

@Slava
Copy link
Member

commented Apr 2, 2015

<div> {{add (add 1 2) 3}} </div>

Implements nested expressions in Spacebars tags.

cc: @stubailo @avital @dgreensp

@Slava

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

Also need tests for unbalanced expressions.

@@ -21,6 +21,7 @@ Tinytest.add("spacebars-compiler - stache tags", function (test) {

run('{{foo}}', {type: 'DOUBLE', path: ['foo'], args: []});
run('{{foo3}}', {type: 'DOUBLE', path: ['foo3'], args: []});

This comment has been minimized.

Copy link
@stubailo

stubailo Apr 2, 2015

Contributor

What is this extra newline dude

}
tag.args.push(newArg);

if (run(/^(?=[\s})])/) !== '')

This comment has been minimized.

Copy link
@stubailo

stubailo Apr 2, 2015

Contributor

I still am not sure what this is supposed to do, let's add a comment

{{/with}}
</template>

<template name="spacebars_template_test_new_each_lookup_top_level">

This comment has been minimized.

Copy link
@stubailo

stubailo Apr 2, 2015

Contributor

These tests aren't supposed to be here!

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2015

I'd rather see parentheses used for JS expressions, and I tend to agree with Avi and Glasser that we shouldn't invent a new Lisp-like DSL, which is the direction this is going in. People will want inline ifs, equality testing, and arithmetic right off the bat, and helper libraries will emerge... it'll be a mess.

Ractive syntax is much more clean and appealing:

<svg viewBox='0 0 100 100'>
  <g transform='translate(50,50)'>
    {{#each segments :i}}
      <polygon
        on-mouseover='set("selected",id)'
        on-mouseout='set("selected",null)'
        class='donut-segment'
        fill='{{colors[id]}}'
        opacity='{{selected ? ( selected === id ? 1 : 0.2 ) :1}}'
        points='{{plot(this)}}'
      >
    {{/each}}

    {{#if selected}}
      <text intro-outro='fade' y='6'>{{points[selected]}}</text>
    {{/if}}
  </g>
</svg>
<div class='rating {{readonly ? "readonly" : ""}}' on-mouseout='highlight(0)'>
  {{#each stars:i}}
    <span class='star {{ rating > i ? "selected" : "" }} {{ highlighted > i ? "highlighted" : "" }}'
          on-tap='select(i+1)'
          on-mouseover='highlight(i+1)'
    >&#9733;</span>
  {{/each}}
</div>

There are definitely back-compat issues with doing exactly with Ractive does, but we could get close.

@Slava

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

@dgreenspan where can I find the discussion by Avi and glasser you are referring to?

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2015

I don't have it in writing, and I don't want to speak for anyone else. We should have a new discussion.

@Slava Slava added the Project:Blaze label Apr 2, 2015

@Slava

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

After the chat conversation between Sashko and Greenspan, we got David (Gsp) onboard for this direction. This change doesn't prevent us from implementing the JS expressions and it is already an official feature of handlebars.

@Slava Slava force-pushed the spacebars-nested-exprs branch 2 times, most recently from c9bfd11 to 87b1fd3 Apr 2, 2015

@Slava Slava force-pushed the spacebars-nested-exprs branch from 87b1fd3 to f6c9ac8 Apr 2, 2015

@Slava Slava force-pushed the spacebars-nested-exprs branch from f6c9ac8 to 1d4bc27 Apr 2, 2015

@Slava

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2015

Cleaned up, added tests and fixed broken tests. No extra newlines, more comments.

@@ -210,6 +218,9 @@ TemplateTag.parse = function (scannerOrString) {
return ['STRING', result.value];
} else if (/^[\.\[]/.test(scanner.peek())) {
return ['PATH', scanPath()];
} else if (/^\(/.test(scanner.peek())) {

This comment has been minimized.

Copy link
@stubailo

stubailo Apr 2, 2015

Contributor

I feel like this should be scanner.run(/\(/) instead of a peek and then pos += 1

@@ -228,6 +228,11 @@ _.extend(CodeGen.prototype, {
case 'PATH':
argCode = self.codeGenPath(argValue);
break;
case 'EXPR':
// The format of EXPR is ['EXPR', { type: 'EXPR', path: [...], args: { ... } }]
var expr = arg[1];

This comment has been minimized.

Copy link
@dgreensp

dgreensp Apr 3, 2015

Contributor

this is already in argValue

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2015

I'd like to see spacebars_tests and template_tests of nested subexprs and subexprs as keyword values.

@Slava

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2015

Got it! Will do.

@Slava

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

@dgreensp @stubailo addressed all comments. Added tests.

@ccorcos

This comment has been minimized.

Copy link

commented Apr 14, 2015

After checking out the template args pull request, I think @dgreensp makes a good point. This looks a lot like Lisp. With @> syntax for arguments as well, I'm worried that the Blaze syntax may become a bit overwhelming especially for new-comers

@Slava Slava merged commit b5642c3 into devel Apr 23, 2015

1 check passed

default The author has signed the Meteor Contributor Agreement.
Details

@Slava Slava deleted the spacebars-nested-exprs branch Apr 23, 2015

@RobertLowe

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2015

@Slava I'm with @ccorcos; I think there needs to be clear guidance (documentation), recently, you've (awesomely) made quite some small/"big" changes to Blaze #4118 #4113

We've now broken away from logic-less templates a bit: ex: {{if (is thing)}}, I mean, at this point, why not just allow for javascript in expressions?

I mean, I like the additions, but I have concerns about this making Blaze further more complex for beginners, data contexts are already a difficult thing to explain, but now we have thrown in nested expressions, named each/with arguments, and named template arguments. I feel they'll further confuse users.

Any thoughts?

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2015

LGTM.

While we are still thinking about introducing JS expressions into templates (I think it's a sound idea), mixing JS and Handlebars expressions in a standard {{}} tag could make things really confusing because of differences between the languages. If we introduce JS expressions, it will probably have to be through another route like a different kind of mustache tag or a larger overhaul of templating.

@RobertLowe

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2015

@Slava Thanks, I see.

It still feels like a turn away from logic-less templates (as mustache did by design) which enforced a developer to separate business logic from the template. With logic-less and no sub-expressions, the units of functionality were small and could be unit tested easily. However with sub-expressions the logic becomes nested in the template and the unit becomes harder to test.

Food for thought.

👍

Slava added a commit that referenced this pull request Apr 23, 2015
@mitar

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2015

Yea!

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2015

Maybe Spacebars README file should be updated with examples of this?

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2015

Does {{foobar (zoo a='42')}} work? Can you pass keyword arguments to nested expression?

@rclai

This comment has been minimized.

Copy link

commented May 15, 2015

How deep do these sub-expressions go (not that I would nest that deep)? Would this work?

<div> {{add (add 1 (add 1 (add 1 (add 1 2)))) 3}} </div>
@rclai

This comment has been minimized.

Copy link

commented May 15, 2015

Also, does this work?

{{> template a=(add 1 2)}}
{{helper a=(add 1 2)}}
@dgreensp

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

@rclai Yes and yes. As deep as you want. :)

@ephemer

This comment has been minimized.

Copy link

commented May 18, 2015

None of this code

<div> {{add (add 1 2) 3}} </div>

or

{{> template a=(add 1 2)}}
{{helper a=(add 1 2)}}

..makes sense to me in terms of syntax or architecture.

I'm assuming these are contrived examples and we'd be expected to put real variables or functions in place of the 1 and 2. Where does it end? It seems to be adding (potentially a lot of) complexity, not taking it away. Also, putting logic in the views seems to be going against the MV* model, and generally against separation of concerns. What is the benefit of this feature?

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented May 18, 2015

I agree the syntax seems a bit odd at first, but think of each parenthesized expression as being a mustache within a mustache. This syntax is part of Handlebars.

One reason subexpressions are important is that they make it easier to use templates to abstract patterns of HTML, so you can replace <span class="label">{{helper arg}}</span> with {{>label text=(helper arg)}}. Expressions like {{#if (isSelected item)}} keep you from having to define extra one-off helpers like isItemSelected, which accesses this.item and will only ever be used from this one place.

There's no such thing as a "logicless view," only a logicless template. You're always going to have presentation logic that calculates what HTML to display (e.g. attributes of HTML tags) or reshapes data from the model (so you can loop over it and display a table). Separation of concerns demands that you separate this logic from your business logic, whether it goes in the template or not. If anything, keeping presentation logic outside the template makes it easier to blur the line with business logic! However, there is no silver bullet for separating concerns.

It's good to be suspicious when a template has too much logic in it, but Handlebars is not a logicless template language (true examples of which are very rare); it has helpers. Letting them nest relieves some awkwardness.

@ephemer

This comment has been minimized.

Copy link

commented May 18, 2015

Somehow these arguments make sense, and I agree that it may be quite possible there's no better way to achieve the same result within the confines of Handlebars. That said, I have a gut feeling that this is the kind of decision we'll wake up to in a couple of years and say, "What the hell were we thinking?", like we do with PHP or other outdated paradigms today.

The question of separation of concerns is tricky. I agree that view-only logic (i.e. logic that has no influence over our data itself or over other parent or sibling views) should remain in the view, but this kind of logic is quite rare. Normally we want to do something with the data we're interacting with (show another template in a Template.dynamic block on click, etc). A prime place for this is in the Template.Name.helpers and .events, of course. We have no way of storing state in the view itself without interacting with our controller anyway, unless we use jQuery.data or CSS tricks.

I personally find the example with {{> label text=(helper arg)}} unconvincing. You could just as easily put the {{helper arg}} inside the {{> label }} template, if you want to do it that way. Even better would be to write {{> label }} as a block helper and put {{helper arg}} inside, i.e.:

{{#label }}{{helper arg}}{{/label}}

It seems to me that you're not saving any time or complexity with the more complex nested expressions syntax, just making the templates harder to understand. Especially when they're nested (the fact that nesting is possible should be a warning sign in itself, I think).

Another way of having more complex logic without full-blown statement support that I have seen is to use an {{#is }} helper, e.g.: {{#is item selected}}. selectedcould even be a reusable (potentially global) helper function that takes the first argument (here, item) as a parameter. I recognise this is slightly less flexible, but seems to cover a lot of the use-cases these nested expressions seem to want to cover, without introducing weird syntax or extra complexity. To me it'd be a more logical addition to the core, in line with separation of concerns and KISS.

@Slava

This comment has been minimized.

Copy link
Member Author

commented May 18, 2015

@RobertLowe I am not going to reply to you here since it is off-topic. You are more than welcome to come to forums and ask the support of the community there. This PR is not the right place for "let's just kill spacebars" posts.

@RobertLowe

This comment has been minimized.

Copy link
Contributor

commented May 18, 2015

@frozeman

This comment has been minimized.

Copy link

commented May 25, 2015

Nice to see this finally implemented!

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2015

It seems I cannot get nested objects to work, so no helper, just an object. I am trying something like:

 {{pathFor 'discussion.new' query=(id='abc')}}
@stubailo

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2015

I don't think the intention of this pr was to add that feature - what would be a new thing.

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2015

So I have to create a dummy helper which just passes it through?

{{pathFor 'discussion.new' query=(passThrough id='abc')}}

This works. But this is really unnecessary.

@stubailo

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2015

I agree it's unnecessary, but what you're asking for is different from nested expressions, you're asking for object literals, I guess.

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2015

Hm, but it feels like it should not be too hard to achieve once we have this now? I will open a ticket.

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2015

There is already ticket for this: #5095

@frozeman

This comment has been minimized.

Copy link

commented Oct 19, 2015

You could accomplish this with a global helper like:

Template.registerHelper('obj', function(obj){
     return obj.hash;
})
{{pathFor 'discussion.new' query=(obj id='abc')}}
@mitar

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2015

Yea, that's what I wrote in #5095. ;-) We moved discussion there.

@mitar

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2015

(But I see that as a workaround.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.