machty / emblem.js

Emblem.js - Ember-friendly, indented syntax alternative for Handlebars.js
http://emblemjs.com
MIT License
1.04k stars 81 forks source link

Support for @-prefixed keywords in the each block helper #150

Closed lai closed 10 years ago

lai commented 10 years ago

This adds the support for data keywords prefixed by @ such as @index, @key in Handlebars' each block helper. Prior discussion about supporting @index at #112

Note that support for @first and @index in object iteration was added in Handlebars in 1.2.0. https://github.com/wycats/handlebars.js/pull/661

lai commented 10 years ago

Sounds good. Does using attributeName here seem reasonable? It's defined as

attributeName = $attributeChar*
attributeChar = alpha / [0-9] /'_' / '-'

So it would just be

eachBlockHelperVariable
  = !'{' _ '@' variableName:attributeName
machty commented 10 years ago

That seems fine to me.

lai commented 10 years ago

I'm writing this test case that uses @first as the if block argument, it seems the syntax is not covered though currently:

// Support for data keywords that are prefixed with @ in the each
// block helper such as @index, @key, @first, @last
eachBlockHelperVariable
  =  _ '@' variableName:attributeName
{
  var params = {};
  var idNode = new AST.IdNode([{part: variableName}]);
  var dataNode = new AST.DataNode(idNode);
  params = dataNode;
  return new AST.MustacheNode([params], null, false);
}

recursivelyParsedMustacheContent
  = !'{' text:$[^}]*
{
  // Force interpretation as mustache.
  // TODO: change to just parse with a specific rule?
  text = "=" + text;
  return Emblem.parse(text).statements[0];
}

mustacheContent
 = eachBlockHelperVariable / recursivelyParsedMustacheContent

Is there a better place to define the eachBlockHelperVariable rather than just as some kind of mustacheContent?

machty commented 10 years ago

Hmm, honestly, this commit should be reverted: https://github.com/machty/emblem.js/commit/f15ffefffe9d71921b4479ed367dca9160128617

There's no reason the parsing of @data keyword should be a sibling of recursive mustache content. It should be parsed like any other mustache-y identifier.

Sorry you've been on this wild goose chase; if you still have energy, and I hope you do, here's what you should do:

1) Revert the above commit 2) Add rules somewhere closer to pathIdNode; pathIdNode is used in a few different places, both as a line-starting mustache, an argument to helpers, and a value for attributes. 3) Keep the test cases you already have as well as the one from the above reverted commit, and add additional ones for the cases described in #2.

The recursiveMustacheParse thing is used in the case that you have something like

p Some text and a #{rescursivelyParsedMustache}

It basically restarts the Emblem parser on everything between the curly braces, but you shouldn't have to touch that in any way to get data vars to work.

It shouldn't be an insane amount of work, but you're correct that support should also exist for using data keywords as helper args, in addition to making the logic universal beyond just @index

lai commented 10 years ago

Tests all passed now for Handlebars 1.3.0 and 1.2.1. Since Handlebars 1.1.2 doesn't have support for these iteration variables, is there any way we can conditionally skip some of the tests?

machty commented 10 years ago

@lai Awesome; yeah, i suppose you can check Handlebars.VERSION to decide whether or not to run that test.

lai commented 10 years ago

@machty Please review the amended commit.

machty commented 10 years ago

Looks good, thank you!

lai commented 10 years ago

Thanks! Can't wait to fully embrace Emblem here :)