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

handlebars mode cannot handle {{#foo}} and {{#if foo}} in the same template, and can't handle {{^foo}} at all #770

Closed
monsanto opened this issue May 30, 2014 · 24 comments

Comments

@monsanto
Copy link
Contributor

http://jsfiddle.net/ezLqe/35/

check console for parse error. can confirm it is supposed to work in tryhandlebarsjs.com

@monsanto
Copy link
Contributor Author

To avoid bug spam: handlebars mode can't handle {{^foo}} sections at all.

http://jsfiddle.net/ezLqe/36/

@monsanto monsanto changed the title handlebars mode cannot handle {{#foo}} and {{#if foo}} in the same template handlebars mode cannot handle {{#foo}} and {{#if foo}} in the same template, and can't handle {{^foo}} at all May 30, 2014
@jo-sm
Copy link

jo-sm commented Jun 16, 2014

I don't think this is an issue with Ractive. Ractive, by default, uses Mustache as its templating system, and Mustache uses {{^foo}} to designate not foo. On the other hand, Handlebars uses {{#if foo}} and {{#unless foo}} as conditionals. Unless I'm not understanding something, this shouldn't work because it's not designed to, since the systems are independent and implement conditionals differently...

@martypdx
Copy link
Contributor

API-wise, I'm not sure one way or the other whether this should work. I'm inclined to say the mustache-based directives should continue to work.

@marcello3d (or maybe @jrajav): Is there any technical reason in the parser can't continue to process mustache-based directives when handlebars: true?

@monsanto
Copy link
Contributor Author

@joshuasmock Handlebars is not an "independent system." It is Mustache, with all of the usual Mustache sections, plus a bunch of other stuff like block sections. If Ractive can't mix Mustache style sections and Handlebars style sections, then Ractive doesn't conform with the official Handlebars compiler.

@martypdx If it is undesirable to allow "mixing", we should document that difference.

@marcello3d
Copy link
Contributor

There's no technical limitation.

Handlebars is described as a super-set of mustache, so it should probably be allowed. On the other handle, we already cannot claim to support any existing mustache/handlebars template, since you're restricted to a DOM tree structure.

My personal feeling is that it is a flaw in handlebars' design to allow mixing of mustache and handlebars-style control blocks because it's a potential source of errors. I'd be all for relegating the restriction to a "strict" mode.

@Rich-Harris
Copy link
Member

I think separating Handlebars mode from strict mode, and allowing mixed styles in non-strict mode, is a good call. IIRC that shouldn't be tricky to implement, we're most of the way there already.

How does everyone feel about enabling Handlebars mode by default, and making strict mode optional? (Or making strict mode the default...? Though that would be a source of breaking changes and confusion for some users who are currently doing things like {{#foo}}...{{/}}.)

@marcello3d
Copy link
Contributor

Unless you want to do a lot of marketing around the change, I'd be hesitant to switch to a handlebars-only strict mode.

I think something like this is better to do gradually. If you feel that the handlebars syntax is better and want to migrate Ractive, I would do the following:

  1. add handlebars support to the "default" mode
  2. update all the documentation/examples to use handlebars-style
  3. add a handlebars-only/handlebars-strict mode that doesn't allow normal mustache {{#blocks}}
  4. possibly add warnings for non-handlebars blocks to nudge users

That might be enough.

@jo-sm
Copy link

jo-sm commented Jun 16, 2014

@monsanto: Since @marcello3d said that Handlebars is described as a super set of Mustache, do both flavors work in conjunction with one another in pure handlebars.js? In other words, will a template with both {{^foo}}...{{/foo}} and {{#if foo}}...{{/if}} work in handlebars.js without any errors?

Like @marcello3d says above, I'd also be hesitant to change template systems completely without a deprecation warning in place, but I also think that supporting both styles simultaneously could cause errors to creep in unexpectedly as well as confusion; I favor being strict in the implementations of both Mustache and Handlebars since Ractive's goal isn't to be a templating engine. However, an interesting idea could be to develop a way to test for and determine the template style, and use that engine without having to declare it. For example, if I have both a Handlebars and a Mustache template, both could work without having to declare which is which because Ractive would know.

@martypdx
Copy link
Contributor

@marcello3d I think for #2, documentation should show both where it makes sense.

Seems like could use a single option like parserMode that had all (default, any falsey), mustache-strict and handlebars. So you can opt-in to preventing one or the other.

@codler
Copy link
Member

codler commented Jun 16, 2014

@martypdx good idea

@marcello3d
Copy link
Contributor

@martypdx to avoid confusion, what about the following 3 modes:

  1. "mustache"
  2. "handlebars" (default, supports both)
  3. "handlebars-only" (doesn't allow mustache-style blocks)

Though, unless there's a strong desire for a mustache-only mode, I'd keep it simple and only support 2 & 3.

@monsanto
Copy link
Contributor Author

@joshuasmock

do both flavors work in conjunction with one another in pure handlebars.js?

Yes, as I said in the first post I can confirm that this is supposed to work, and pointed to tryhandlebarsjs.com (which is using the latest release of Handlebars) so you can see for yourself

To be clear, I don't really care if you can mix the two. It would have been handy when porting from Mustache -> Handlebars, because I wouldn't have had to do it all at once, but for newer users I think we should just recommend using Handlebars from the start.

@jo-sm
Copy link

jo-sm commented Jun 17, 2014

@monsanto Oh, wow, sorry! I completely missed that.

@Rich-Harris I would say that since handlebars.js accepts Mustache templates by default, then I think two modes would be best:

  1. Handlebars mode, by default (parses whatever handlebars.js parses)
  2. Mustache mode (only parses Mustache templates if someone needed it)

I would guess that since Ractive produces these errors where handlebars.js doesn't, then Ractive doesn't use handlebars.js. Would it be possible to port or drop-in handlebars.js for use within Ractive, barring size concerns?

@martypdx
Copy link
Contributor

I don't think Ractive should be preferential towards handlebars. We don't really support handlebars, we just borrowed some handy mustache syntax extensions.

What if we treat this issue as a bug, fix the parser to handle both, and do this release without an option?

Then we see how people use it, what comments and issues we see, then make adjustments in future release.

Would it be possible to port or drop-in handlebars.js for use within Ractive

@joshuasmock handlebars compiles to javascript functions that return a string, so they wouldn't be useful for two-way binding.

@Rich-Harris
Copy link
Member

do this release without an option

I'd be in favour of that. Only remaining question then would be what strict mode entails - does it just mean disallowing unspecified section types (i.e. {{#foo}}...{{/foo}} instead of {{#if foo}}...{{/if}} and so on) and (as a corollary) enforcing the correct closing tag (i.e. {{/if}} as opposed to {{/lol whatever}})? @marcello3d do you have any thoughts on this?

@martypdx
Copy link
Contributor

what strict mode entails

I missed that one, it never made it into up into the declared ractive options. :)

Looking at the code, it's only used here to figure out the closing tag:

if ( parser.options.strict || parser.handlebars ) {
    switch ( mustache.n ) {
        case types.SECTION_IF:
            expectedClose = 'if';
            break;
...

It seems to me that we could just enforce this if one of the handlebars extensions is used to open the tag. Then we don't need that option either. But @marcello3d should chime in.

@martypdx
Copy link
Contributor

FYI - If I comment out that if line, all the tests still pass. Oops, not so.

@marcello3d
Copy link
Contributor

Enforcing closing tag catches bugs earlier and makes templates more readable, so I think we should continue requiring it for the handlebars-if/with/unless/each blocks.

As for strict mode/mismatched mustache-style blocks, it's not always clear what the appropriate close string should be, so I say hold off. (Perhaps strict mode will mean eliminating mustache-style blocks entirely—but I don't think we're ready to make that call.)

@martypdx
Copy link
Contributor

Enforcing closing tag catches bugs earlier and makes templates more readable, so I think we should continue requiring it for the handlebars-if/with/unless/each blocks.

Right, so we always check for if/with/unless/each, meaning we don't need strict mode for now.

@jo-sm
Copy link

jo-sm commented Jun 17, 2014

I don't think that a strict mode would be necessary if the major differences are the extended template syntax that Handlebars provides on top of Mustache, in this case the addition of {{#if}}, {{#else}} etc, as well as paths ({{../person.age}}), since I would think the case for someone to force Mustache only would be small outside of possibly debugging since standard Mustache templates would already work with no changes. I do think that making sure that enforcing closing tags is good for the reasons @marcello3d stated.

@marcello3d
Copy link
Contributor

@joshuasmock I was actually suggesting strict mode would prevent mustache blocks without an explicit handlebars-style if/unless/each/with

@jo-sm
Copy link

jo-sm commented Jun 17, 2014

@marcello3d Oh. But doesn't Handlebars parse {{^foo}}...{{/foo}} as well as {{#if foo}}...{{else}}...{{/if}}?

@marcello3d
Copy link
Contributor

@joshuasmock Yes, but I think it's a bad design.

Rich-Harris added a commit that referenced this issue Jun 17, 2014
@Rich-Harris
Copy link
Member

Handlebars-style blocks are now always supported: http://jsfiddle.net/rich_harris/DQa4p/ There's no handlebars or strict option - if you use a {{#if foo}} opening tag, you have to use an {{/if}} closing tag, but if you use {{#foo}} then {{/random}} will continue to close the section.

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

No branches or pull requests

6 participants