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

Add @value to context when iterating over maps #1253

Closed
wants to merge 2 commits into from

Conversation

giflw
Copy link

@giflw giflw commented Sep 22, 2016

Add @value to context to more semantic the access to map key/value pairs. In this way those both approach will hold the same result, but the second one beeing more semantic:

{{#each myObject}}
    Key: {{@key}} Value = {{this}}
{{/each}}
{{#each myObject}}
    Key: {{@key}} Value = {{@value}}
{{/each}}

Add @value to context to more semantic the access to map key/value pairs. In this way those both approach will hold the same result, but the second one beeing more semantic:

{{#each myObject}}
    Key: {{@key}} Value = {{this}}
{{/each}}

{{#each myObject}}
    Key: {{@key}} Value = {{@value}}
{{/each}}
@ErisDS
Copy link
Collaborator

ErisDS commented Sep 28, 2016

Just throwing my 2c in to say 1) I agree with the premise that @value is more semantic. It's syntactic sugar for sure, but I can see how this could significantly increase template readability.
2) Implementation also looks good, can't think of any unintended side affects - Ghost has it's own loop helper in which we've done quite a few similar extensions and not found any downsides.

I do think this warrants testing though, if possible?

@stevenvachon
Copy link

I do think this warrants testing though, if possible?

Absolutely agree. Who adds new features without writing tests? @giflw? 😛

@giflw
Copy link
Author

giflw commented Sep 28, 2016

Absolutely agree! I'll do as soon as possible. 😄

@giflw
Copy link
Author

giflw commented Sep 28, 2016

Tests done! Please let me know if needs more tests. 😄

@stevenvachon
Copy link

Nice! Looking forward to this one.

@nknapp nknapp added the feature label Jan 1, 2017
@giflw
Copy link
Author

giflw commented Jan 8, 2018

Hi! Is there a planned version to include this PR @nknapp ?

@nknapp
Copy link
Collaborator

nknapp commented Jan 10, 2018

That would be @wycats decision (new language feature).

@wycats
Copy link
Collaborator

wycats commented Jan 11, 2018

@giflw @nknapp I have no problem, in principle with adding this feature to the current feature set.

However, I strongly believe that this set of features is best achieved in practice using the block-param feature:

{{#each myObject}}
    Key: {{@key}} Value = {{this}}
{{/each}}

{{#each myObject as |key value|}}
    Key: {{key}} Value = {{value}}
{{/each}}

The Ember dialect of Handlebars leans heavily on the block-params pattern and it has proven to be quite versatile. Here's an example from the Ember documentation:

<ul>
  {{#each-in categories as |category products|}}
    <li>{{category}}
      <ol>
        {{#each products as |product|}}
          <li>{{product}}</li>
        {{/each}}
      </ol>
    </li>
  {{/each-in}}
</ul>

It is going to make sense for us to see about re-converging Handlebars with the exploration that went into the Ember dialect of Handlebars. Given the relative size of the Handlebars community, we will want to be very careful about any movement we make in that direction, so for the moment, I have no problem with this proposal.

@nknapp
Copy link
Collaborator

nknapp commented Jan 11, 2018

I'm a bit confused now. @wycats you say you have no problem with the feature because you want to converge with ember style loops (block params)?
Wouldn't this be a reason not to implement this feature?
Given the fact that the block-params are alreadyv working in the "each"-helper, I would say we don't actually need the @value anymore.

@wycats
Copy link
Collaborator

wycats commented Jan 11, 2018

@nknapp if we're comfortable telling people to use the new block params form from now on, there's no need to add this feature 😄

I just didn't know if I could make a command decision like that in this thread, which is really about what seems like a very minor feature addition.

@giflw
Copy link
Author

giflw commented Jan 11, 2018

I still think this is a good addition, as it's simple than the block syntax.

@wycats
Copy link
Collaborator

wycats commented Jan 11, 2018

@giflw when you have nested loops, it becomes more complicated than the block syntax:

{{#each posts}}
  <h1>{{@key}}</h1>

  <div>{{@value}}</div>

  <h2>Comments</h2>

  {{#each comments}}
    <h3>Reply to {{@key}} {{! oh no, @key has been overridden 😱 }}</h3>
  {{/each}}
{{/each}}

vs. the block syntax:

{{#each posts as |title body|}}
  <h1>{{title}}</h1>

  <div>{{body}}</div>

  <h2>Comments</h2>

  {{#each comments as |commentTitle commentBody|}}
    <h3>Reply to {{title}} {{! no problem I can easily refer to the outer title }}</h3>
  {{/each}}
{{/each}}

This kind of thing is why the block param syntax was added. It might take some getting used to, but it has no surprising "gotchas" when nesting things.

@nknapp
Copy link
Collaborator

nknapp commented Jan 14, 2018

I am not sure about this, but I tend to say: Use block syntax and do not add the @value.

It looks like a harmless addition on the first glance, but when I think about it: Strictly speaking, it is a breaking change.

If someone has written a custom block helper that sets @value, and uses the {{#each}} helper inside the custom-helper's block, the @value would get overridden as well. Something like this:

{{#myCustomHelper}} {{! sets "@value" }}
  Value: {{@value}}
  {{#each anObject}}
    {{@value}}: {{this}}<br>
  {{/each}}
{{/myCustomHelper}}

We could merge it into master, but I don't know when the 5.0 version will see the light of the day. And I would not merge it into the 4.x.

I am reluctant to merge this, especially when there is another syntax to achieve the same goal.

@wycats
Copy link
Collaborator

wycats commented Jan 15, 2018

@nknapp Good point!

And in fact, the motivation for block params is precisely the fact that this is a breaking change!

I'm going to close this.

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

Successfully merging this pull request may close these issues.

5 participants