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

Nested each with equal values skips some iterations. #1686

Open
graphicore opened this issue May 5, 2020 · 5 comments
Open

Nested each with equal values skips some iterations. #1686

graphicore opened this issue May 5, 2020 · 5 comments
Labels

Comments

@graphicore
Copy link

Before filing issues, please check the following points first:

This will probably help you to get a solution faster.
For bugs, it would be great to have a PR with a failing test-case.

Here's the jsfiddle for the issue: https://jsfiddle.net/graphicore/ge1qxvc8/

Template

_{{#each abc as | c1 |}}{{#each ../abc as | c2 |}}{{#each ../../abc as | c3 |}}{{c1}}{{c2}}{{c3}} {{else}}! {{/each}}| {{/each}}{{/each}}_

Data

{
    abc: ['A', 'B', 'C']
}

Observed Behavior

_! | ABA ABB ABC | ACA ACB ACC | BAA BAB BAC | ! | BCA BCB BCC | CAA CAB CAC | CBA CBB CBC | ! | _ 

Expected Behavior

_AAA AAB AAC | ABA ABB ABC | ACA ACB ACC | BAA BAB BAC | BBA BBB BBC | BCA BCB BCC | CAA CAB CAC | CBA CBB CBC | CCA CCB CCC | _ 

Context:

I'm porting a font testing tool written mostly in server side PHP to client side JavaScript. For some samples I need to generate grids of char combinations e.g. to let a designer test spacing uc-spacing-03.php.

I believe 279e038 is related (commit message).

graphicore added a commit to graphicore/handlebars.js that referenced this issue May 5, 2020
graphicore added a commit to graphicore/handlebars.js that referenced this issue May 5, 2020
graphicore added a commit to graphicore/handlebars.js that referenced this issue May 5, 2020
graphicore added a commit to graphicore/handlebars.js that referenced this issue May 5, 2020
…1686

According to the commit message of 279e038:

" [...] Helper authors now need to take care to return the same context
value whenever it is conceptually the same and to avoid behaviors that
may execute children under the current context in some situations and
under different contexts under other situations."

I belive that metric is too implicit, as the issue demonstrates. This
is because in this case the context value is not conceptually the same,
but it is de facto the same value.

Instead this suggests to use an explicit option flag "goDeeper" to mark
when a helper definitely should go deeper. This also should be backwards
safe, as it doesn't change the behavior of existing helpers, builtin or
custom, except the builtin "each",
graphicore added a commit to graphicore/handlebars.js that referenced this issue May 5, 2020
Move test case into parent test suite "regressions".
graphicore added a commit to graphicore/handlebars.js that referenced this issue May 5, 2020
…1686

According to the commit message of 279e038:

" [...] Helper authors now need to take care to return the same context
value whenever it is conceptually the same and to avoid behaviors that
may execute children under the current context in some situations and
under different contexts under other situations."

I belive that metric is too implicit, as the issue demonstrates. This
is because in this case the context value is not conceptually the same,
but it is de facto the same value.

Instead this suggests to use an explicit option flag "goDeeper" to mark
when a helper definitely should go deeper. This also should be backwards
safe, as it doesn't change the behavior of existing helpers, builtin or
custom, except the builtin "each",
@graphicore
Copy link
Author

I added a test and suggested a fix in PR #1687

@nknapp
Copy link
Collaborator

nknapp commented May 5, 2020

Is this a duplicate of #1300 ?

@graphicore
Copy link
Author

@nknapp I looked at that issue before, but it's just now that I've noticed your workaround, and indeed it works.

https://jsfiddle.net/graphicore/shn9oyb6/1/

{{#with abc as  | abc |}}
	_{{#each abc as | c1 |}}{{#each abc as | c2 |}}{{#each abc as | c3 |}}{{c1}}{{c2}}{{c3}} {{else}}! {{/each}}| {{/each}}{{/each}}_
{{/with}}

It even gets rid of the ../ parent scope selectors, which I quite like.

I have to admit, the scoping concept of handlebars.js is a bit mysterious to me. If this would work without the {{#with abc as | abc |}} I'd understand the concept as falling back to the parent scope when a name is undefined in the current scope and ../ to make shadowed names from parent scopes accessible, but having to use with to achieve this is unexpected.

FWIW, this remains broken:

{{#with abc as  | abc |}}
_{{#each ../abc as | c1 |}}{{#each ../../abc as | c2 |}}{{#each ../../../abc as | c3 |}}{{c1}}{{c2}}{{c3}} {{else}}! {{/each}}| {{/each}}{{/each}}_
{{/with}}

@nknapp
Copy link
Collaborator

nknapp commented May 7, 2020

Not adding a scope when the context remains the same is something that was introduced in 4.0 and I'm not happy with it either. I actually like your solution but I think it should be completely dependent on the helper and not on whether the context changes. But this would be a breaking change, which is why I haven't attempted to implement it so far.

@graphicore
Copy link
Author

but I think it should be completely dependent on the helper and not on whether the context changes. but I think it should be completely dependent on the helper and not on whether the context changes.

I agree, I kept the old condition to stay compatible. It could be removed for version 5, if the project policy allows that.

If it's possible to introduce bigger changes I may suggest another one, in another issue.

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

No branches or pull requests

3 participants