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

"unless" breaks when "each" value equals "null" #1319

Closed
jeremykentbgross opened this issue Mar 3, 2017 · 14 comments · Fixed by renovatebot/renovate#194
Closed

"unless" breaks when "each" value equals "null" #1319

jeremykentbgross opened this issue Mar 3, 2017 · 14 comments · Fixed by renovatebot/renovate#194

Comments

@jeremykentbgross
Copy link

When using "each" over either an object or an array and a null value is encountered, "unless" will destroy the context, replacing the null value with an empty object, and dropping parent access all together.

Here is a reproduction test case: https://jsfiddle.net/r33cjjb2/3/

To aid seeing the issue, the failing case renders the following inline: !!!FAIL!!!FAIL!!!FAIL!!!FAIL!!!FAIL!!!FAIL!!!FAIL!!!FAIL!!!FAIL!!!FAIL!!

Looking forward to seeing this fixed. It is a real pain in my @$$ atm. Otherwise thank you for a magnificent tool.

@jeremykentbgross
Copy link
Author

It seems that if/else have the same bug. Updated test case: https://jsfiddle.net/r33cjjb2/4/

@nknapp
Copy link
Collaborator

nknapp commented Mar 4, 2017

I tried to reproduce this in a simpler jsfiddle https://jsfiddle.net/r33cjjb2/6/. What you mean is, that the first line of output of my jsfiddle should have parent="parent value" in it, right?

@jeremykentbgross
Copy link
Author

Hey nknapp,

Sorry my example wasn't as simple as yours. It was a stripped down version of my actual use case, But yes that is correct, except that actually it's a bit worse than that. Yours only captures part of the issue.

I updated yours again:
https://jsfiddle.net/r33cjjb2/9/

Note that the first (relevant) line (after the 'unless' header) now reads as:
this="[object Object]" parent=""

(The object, as you will see if you catch it in a helper or something as I did for debugging my real project, contains no members.)

It should match the 'no unless' case (I think), which comes out correctly as:
this="" parent="parent value"

As it is, all context values:

  • 'this',
  • '.' (ie 'this' again via dot),
  • '..' (parent)

are containing unexpected values that are inconsistent with all other cases. If the unless is not present, you would get the correct result (as the updated example, or my original shows). In every other case they match.

Also not included in this simpler version is the fact that {{else}} cases are also broken (if memory serves). I also have not tested {{#if}}{{else}}{{/if}}, but looking a bit at the handlebars source code (which I did on my phone on the train the day i submitted this), suggests it probably also has the same issue.

Thanks for your time.

-Jeremy

@jeremykentbgross
Copy link
Author

Correction: I forgot that I DID test if/else case, the fiddle for it I had already posted above your first reply.

@jeremykentbgross
Copy link
Author

Also I don't know if it is relevant or not, but my original one demonstrated that this was true for iterating over either objects or arrays. Your example only shows arrays. Perhaps this is enough test coverage for the problem, I don't know the internal implementation really so I error-ed on the side of caution and inclusiveness. Sorry if it was too much.

@nknapp
Copy link
Collaborator

nknapp commented Mar 8, 2017

My example did not cover all aspects, that's right. I just wanted to check if I understood the problem correctly. No problem.

I have to dig into the code as well. It seems like the "unless"-helper just calls the if-helper with flipped blocks.

@nknapp
Copy link
Collaborator

nknapp commented Mar 8, 2017

One thing I noticed is that for the null-context, in this line the context-parameter is not null but {}. However depths[0]=null which is why the context is added to the currentDepths. This is essentially the reason why the currentDepths are shifted and return the wrong value.

@nknapp
Copy link
Collaborator

nknapp commented Mar 9, 2017

The place where {} is passed to the function instead of null is here
The change was introduced due to #1093.

nknapp added a commit that referenced this issue Mar 9, 2017
nknapp added a commit that referenced this issue Mar 9, 2017
Fixes #1319

Original behaviour:
- When a block-helper was called on a null-context, an empty object was used
  as context instead. (#1093)
- The runtime verifies that whether the current context equals the
  last context and adds the current context to the stack, if it is not.
  This is done, so that inside a block-helper, the ".." path can be used
  to go back to the parent element.
- If the helper is called on a "null" element, the context was added, even
  though it shouldn't be, because the "null != {}"

Fix:
- The commit replaces "null" by the identifiable "container.nullContext"
  instead of "{}". "nullContext" is a sealed empty object.
- An additional check in the runtime verifies that the context is
  only added to the stack, if it is not the nullContext.
nknapp added a commit that referenced this issue Mar 9, 2017
Fixes #1319

Original behaviour:
- When a block-helper was called on a null-context, an empty object was used
  as context instead. (#1093)
- The runtime verifies that whether the current context equals the
  last context and adds the current context to the stack, if it is not.
  This is done, so that inside a block-helper, the ".." path can be used
  to go back to the parent element.
- If the helper is called on a "null" element, the context was added, even
  though it shouldn't be, because the "null != {}"

Fix:
- The commit replaces "null" by the identifiable "container.nullContext"
  instead of "{}". "nullContext" is a sealed empty object.
- An additional check in the runtime verifies that the context is
  only added to the stack, if it is not the nullContext.
nknapp added a commit that referenced this issue Mar 9, 2017
Fixes #1319

Original behaviour:
- When a block-helper was called on a null-context, an empty object was used
  as context instead. (#1093)
- The runtime verifies that whether the current context equals the
  last context and adds the current context to the stack, if it is not.
  This is done, so that inside a block-helper, the ".." path can be used
  to go back to the parent element.
- If the helper is called on a "null" element, the context was added, even
  though it shouldn't be, because the "null != {}"

Fix:
- The commit replaces "null" by the identifiable "container.nullContext"
  instead of "{}". "nullContext" is a sealed empty object.
- An additional check in the runtime verifies that the context is
  only added to the stack, if it is not the nullContext.
@jeremykentbgross
Copy link
Author

Thanks for your attention on the matter.

Do you know how long it will take for the fix to turn up in the npm and bower packages? Do I need to wait for a version number increase? If so which one should I expect it will turn up in?

nknapp added a commit that referenced this issue Mar 25, 2017
Fixes #1319

Original behaviour:
- When a block-helper was called on a null-context, an empty object was used
  as context instead. (#1093)
- The runtime verifies that whether the current context equals the
  last context and adds the current context to the stack, if it is not.
  This is done, so that inside a block-helper, the ".." path can be used
  to go back to the parent element.
- If the helper is called on a "null" element, the context was added, even
  though it shouldn't be, because the "null != {}"

Fix:
- The commit replaces "null" by the identifiable "container.nullContext"
  instead of "{}". "nullContext" is a sealed empty object.
- An additional check in the runtime verifies that the context is
  only added to the stack, if it is not the nullContext.

Backwards compatibility within 4.0.x-versions:
- This commit changes the compiler and compiled templates would not work
  with runtime-versions 4.0.0 - 4.0.6, because of the "nullContext"
  property. That's way, the compiled code reads
  "(container.nullContext || {})" so that the behavior will degrade
  gracefully with older runtime versions: Everything else will work
  fine, but GH-1319 will still be broken, if you use a newer compiler
  with a pre 4.0.7 runtime.
@nknapp
Copy link
Collaborator

nknapp commented Mar 25, 2017

I have made a slight change to my commit. In the previous version, templates compiled by a newer compiler would not work with the runtimes that a current publish (4.0.0 - 4.0.6). The latest change adds some compatibility code, which does not solve this bug (if you are using a runtime < 4.0.7) but at least it does not break anything else.

This resolves my concerns that this might be a breaking change and since noone has felt obligated for a code-review I will just merge the changes into the 4.x and the master-branch.

However, I cannot publish anything to the repositories and I don't know when anybody has the time (see #1312 ). It's not in my power to do that.

@nknapp
Copy link
Collaborator

nknapp commented Apr 27, 2018

I can see that I have made a typo in the commit message. It should read

This commit changes the compiler and compiled templates would not work
with runtime-versions 4.0.0 - 4.0.6, because of the "nullContext"
property. That's why, the compiled code reads
"(container.nullContext || {})" so that the behavior will degrade
gracefully with older runtime versions: Everything else will work
fine, but GH-1319 will still be broken, if you use a newer compiler
with a pre 4.0.7 runtime.

What I wanted to write was: I had a solution ready, but it would have been breaking. So I added an additional check to the compiler code that would allow the templates to work with older runtimes. In order to get the bugfix however, you'll need a new runtime and compiler.

Did you encounter a problem were runtime and compiled template were incompatible?

@rcampbel
Copy link

@nknapp : Sorry it has taken so long to get back about this, I just had a better chance to dig into what is happening, and it is not related to this change.

I will be opening a different issue/commenting on the change that introduced the issue I'm seeing, once I can get a code example that can be publicly shared.

I will be deleting the comment that I made earlier in this thread in error.

@DevFacts
Copy link

DevFacts commented May 8, 2020

I had been facing the same issue while checking conditional statements in the DOM section to hide/show the content.

So, I added this script in the backend as Handlebars Helpers.
And solved my issues

Handlebars.registerHelper('iff', function(a, operator, b, opts) {
var bool = false;
switch(operator) {
case '==':
bool = a == b;
break;
case '>':
bool = a > b;
break;
case '<':
bool = a < b;
break;
case '!==':
bool = a !== b;
break;
case '===':
bool = a === b;
break;
default:
throw "Unknown operator " + operator;
}

if (bool) {
    return opts.fn(this);
} else {
    return opts.inverse(this);
}

});

Uses example

{{#iff data.value "===" 45 }}
Your DOM goes here ...
{{/iff}}

@elonzh
Copy link

elonzh commented Oct 13, 2020

still have the issue in v4.7.6, it should be reopened.

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

Successfully merging a pull request may close this issue.

5 participants