Conditionals affect ../ path segment behaviour in blocks

[ErisDS]

I'm not sure if this is a bug or not, but it seems very unexpected:

At the top level of the context tree, assuming an object a bit like {a: 1, b: 2}

{{#if a}}
<p>Output: {{b}}</p>
{{/if}}   

It is expected that {{b}} would become 2.

However, when inside the scope of an object and using ../ path segments to reference the parent template, things get a bit weird if you also add a conditional into the mix:

Assume an object like {a: {b: true}, c: 'd'}

{{#a}}
    <p>Output: {{../c}}</p>
  {{#if b}}
    <p>Output: {{../c}}</p>
  {{/if}}
{{/a}}   

I would expect the two items above to both output the same... however in order to output c, it requires two ../ segments - which I don't understand :(

{{#a}}
  {{#if b}}
    <p>Output: {{../../c}}</p>
  {{/if}}
{{/a}}   

Did I find a bug or am I missing something?

Fiddle with the above examples in detail: http://jsfiddle.net/ErisDS/618njbng/

[kpdecker]

Each block creates a context. So basically every time you do {{# you need to add a ../ to access the same data. This is made confusing by some helpers that base the same context so {{this}} and {{..}} resolve to the same object.

We choose to have the simple rule above as it's easier to reason about from the static source code, both for the compiler and the programmer, assuming they are aware of the rule.

This is not the first time similar concerns have been raised, I'm just not sure if the cost of breaking changes and performance impact is less than the conceptual overhead that users have to be aware of for this rule.

[ErisDS]

That makes sense in and of itself, because some, but not every{{#}} changes the context. In particular you can (and in Ghost we do) create a custom block helper that does change it, or one that doesn't.

So I can see how the rule makes sense, however there are two problems with it:

First: the behaviour is inconsistent - if what you're saying is true, then surely my very first example should be:

{{#if a}}
<p>Output: {{../b}}</p>
{{/if}}  

Or is that what you're referring to about {{this}} and {{..}} resolving the same? (Sorry that bit went a slightly over my head). This inconsistency is the major reason for me raising the issue - I'm trying to build good example documentation, and don't know what to do with this!

Secondly, the documentation on handlebarsjs.com is either wrong, or using very confusing language:

The ../ path segment references the parent template scope, not one level up in the context. This is because block helpers can invoke a block with any context, so the notion of "one level up" isn't particularly meaningful except as a reference to the parent template scope.

It seems to me that ../ behaves as though it does reference one level up in the context, and not the parent template scope. In my example, the parent template scope is the same regardless of whether you're inside a conditional or not?

I am interpreting 'parent template scope' to mean the root of the JSON object associated with the template you're currently working in. The way I'm reading that documentation is that ../ doesn't do what it does in other scenarios (file paths) in that it resets to the root, rather than going back one level ... i.e. it's more akin to doing cd / as opposed to cd ../.

[kpdecker]

One way to think about it is that this is an array, literally depths in the generated code, that is unshifted on exec of each helper and then ../ is used to point to a different entry in that array.

If you had something like:

{{#each foo}}
  {{#if bar}}
    {{baz}}
  {{/if}}
{{/each}}

The inner most block would be executing against depths stack of something like:
depths = [initialContext.foo[0], initialContext.foo[0], initialContext]

So {{baz}} compiles to something roughly like depths[0].baz. {{../baz}} to depths[1].baz and {{../../baz}} to depths[2].baz. Since depths[0] === depths[1], {{baz}} and {{../baz}} will render the same thing for this given set of helpers. That would not be true if if were each or another helper that changes the context.

I agree that the documentation you cited is quite confusing. It would be more accurate to say that it references the parent block rather than parent template but I don't know if that's any clearer to a user who is not familiar with the innards of the library. I can try to fix although clear accessible documentation is not always my strong suit :)

[mattkime]

i'd like to cast my vote for #if not changing context. i understand why its this way, etc. but i think the naive assumption about how it works would be preferred.

[ErisDS]

I'm thinking along the same lines. I'm loathe to say it because I fully understand what a pain in the ass it will be to change, but really - I think the behaviour here is wrong.

@kpdecker your last explanation made the what and why make 100% sense to me. Thanks for taking the time to do that, I really appreciate it.

Still even from just the perspective of documenting this - it's nigh impossible to explain without giving away the internals, which is a huge indicator that the behaviour is not ideal.

Handlebars' power lies in that it is so intuitive - 99.9% of the time it just works in that it does exactly what you expect it to do. That means that when you find the 0.1% where it trips you up and does something unexpected, the learning curve is enormously steep because there was no learning required at all in order to start using it.

That makes me in favour of moving that 0.1% as far away from standard use cases as possible.

Therefore, I'd suggest that at the very least #if and #unless be treated as special cases that do not increase depth.

Being able to make the same thing happen with custom block helpers would be a nice to have, but it's still a use case which is significantly further from the average user than {{#if}}.

I suppose that the real solution is to have a more intelligent data structure for handling depth? I'd need to look at the code more closely. I'm very much up for helping out if this change gets the go-ahead.

If not, I'd also be willing to have a go at documenting it more clearly. I'm not great at docs either but maybe my being an outsider will help.

[kpdecker]

On the surface, the technical side of supporting something like this across the board is not much:
https://github.com/wycats/handlebars.js/blob/master/lib/handlebars/runtime.js#L177 would need to have something like:

    let callDepths = depths;
    if (depths && depths[0] !== context) {
      callDepths = [context].concat(depths);
    }

And it should work relatively transparently. There are two downsides to this:

1. Massively breaking change. Any user of ../ will now have to evaluate that their code still works as expected. This task is complicated by (2).
2. A template's behavior is now dependent on the implementation of the helper. Basically I don't know what ../ will resolve to without knowing what the helper does. If the implementation of a given helper were overridden to provide a slightly altered context, templates have the chance to break through no fault of their own, reducing portability.

Basically the implementation right now is using an initially surprising rule that is consistent at all times, ../ is used to step out of a #. The implementation I talked about above is much friendlier for the way that people think but has the potential for not being consistent between different executions, which could lead to bugs. Per the norm, one way or the other people won't be happy with me ;)

I really do like the "friendlier" solution but am having trouble getting over the risk factors above.

[ErisDS]

I have been thinking about this, and have a couple of points to add:

A template's behavior is now dependent on the implementation of the helper. Basically I don't know what ../ will resolve to without knowing what the helper does.

I'd suggest that this is no worse than the current implementation. At the moment the behaviour of ../ is largely undocumented. It doesn't conform to the single statement that the documentation does have, and it doesn't operate intuitively because it's different to the way that ../ operates elsewhere.

Another way to look at it is that if I specify ../ I definitely intended to change the context. If that's not what happens, then I think we can reasonably assert that it's unexpected behaviour?

All of this being said, I have absolutely no idea how I'd manage this change in Ghost themes, it's going to cause absolute havoc for sure!

[kpdecker]

I recently used ../ on a project a bit more than normal and it's quite a pain to use in the current implementation. I think it might be worth the cost to change this in the next major version.

Basically we would make the push operations for depths conditional and it would only occur if the context changes, following my comment above.

For the upgrade path, this is something that is grepable and in theory has less use than other things so it might not be that painful. Regardless we should have a flag that allows the user to choose. (We could implement this behind a flag without a major change and then switch the default value's meaning in the next major release but I'm not sure that that is worth the overhead since we try to avoid major releases and that could be a ways off)

[ErisDS]

Just for interest's sake, the thing that made me raise this originally, was I wanted to do some work to make 'Casper', the default theme for Ghost, more consistent. I wanted to change this block in the tag template to be a {{#tag}}{{/tag}} blog, just like the equivalent author block in the author template.

However, because of the if statement in the tag template, the snippet: A {{pagination.total}}-post collection would need to change to A {{../../pagination.total}}-post collection making it no less consistent and if anything, way more confusing. So I didn't make the change.

The upgrade path is difficult, of course. I think if there's going to be a flag to switch the behaviour for a version, then that is going to help to smooth the transition. Given that the behaviour is both unintuitive according to normal ../ behaviour and also not doing what the documentation states it should, I'd suggest that makes this a bug fix, even though it is a also breaking change.

I was wondering, if it is implemented such that too many ../'s (i.e. you go past the root) just stops at and returns the root value, would that mitigate a significant portion of potential breakages?

E.g. in my case I want to go back to the template root (which is what the documentation suggests a single ../ is intended to do. Given that I am inside a block and an if statement I therefore had to go back up 2 levels to get a root property. With this change there is only one level, meaning my code is wrong, but if it stops at or defaults to the root, I still get my desired effect? My code is wrong, not broken.

[waynedpj]

👍 for the breaking but IMHO more intuitive change. this has bitten me a few times as a new Handlebarser. i think @ErisDS hit it on the head

if I specify ../ I definitely intended to change the context.

thanks for posting and considering.

[kpdecker]

@ErisDS @root exists for cases that people want explicitly access the root. The behavior for out of bounds depth references right now is to omit the value like any other undefined/missing value (or throw under strict)

    return this.escapeExpression(this.lambda((depths[1] != null ? depths[1].foo : depths[1]), depth0));

I don't want to go down the route where people start trying to throw in an arbitrary number of ../ references and hope that they hit the arbitrary root object. At least with the same helper behavior that I implemented, you should be able to reason about what is going on based on each helper that is used.

This is now implemented in master and I'd appreciate if the interested parties could take a look and give me a sense of how painful their own implementations are. When I write the release notes, I plan on saying that the migration path involves checking all use of ../ but no other templates need to be altered.