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

Comparison filters (@eq, @ne, etc.) should accept undefined as a key. #59

Closed
smfoote opened this issue Dec 4, 2013 · 8 comments · Fixed by #105
Closed

Comparison filters (@eq, @ne, etc.) should accept undefined as a key. #59

smfoote opened this issue Dec 4, 2013 · 8 comments · Fixed by #105

Comments

@smfoote
Copy link
Contributor

smfoote commented Dec 4, 2013

If I have a template where a certain key in the JSON may or may not be defined, and I use a comparison filter, the body and else blocks are skipped.

Example JSON

{
  "myArray": [
    {
      "myOptionalCount": 0,
      "name": "Nothing here"
    },
    {
      "name": "Even less here"
    }
  ]
}

Example template

{#myArray}
  for {name}
  {@ne key=myOptionalCount value=0}
    myOptionalCount is not 0
  {:else}
    myOptionalCount is 0
  {/ne}
  {~n}
{/myArray}

Example output

for Nothing here myOptionalCount is 0
for Even less here

Expected output

for Nothing here myOptionalCount is 0
for Even less here myOptionalCount is not 0
@prashn64
Copy link
Contributor

prashn64 commented Mar 5, 2014

+1, this seems really broken right now.

It will probably just be a fix to the filter function.

@salazarm
Copy link

salazarm commented Mar 6, 2014

I would use

  {?myOptionalCount}
    myOptionalCount is not 0
  {:else}
    myOptionalCount is 0
  {/myOptionalCount}

@prashn64
Copy link
Contributor

prashn64 commented Mar 6, 2014

I'd say that is a work around, but it'd have to be more like this:

{?myOptionalCount}
  {@ne key=myOptionalCount value=0}
    myOptionalCount is not 0
  {:else}
    myOptionalCount is 0
  {/ne}
{:else}
  myOptionalCount is not 0
{/myOptionalCount}

The problem gets a lot worse the bigger the markup that would replace "myOptionalCount is not 0" gets.

@salazarm
Copy link

salazarm commented Mar 6, 2014

{?myOptionalCount}

evaluates to false when myOptionalCount is 0 so I don't think you need the @ne

@prashn64
Copy link
Contributor

prashn64 commented Mar 6, 2014

Good point. A better value to illustrate the bug would be one that is greater than 0.

@k2s
Copy link

k2s commented Mar 6, 2014

example how to solve this problem with @js helper

@kate2753
Copy link
Contributor

kate2753 commented Mar 7, 2014

At first glance it seems reasonable to output {:else} block in this case. But there are few edge cases to keep in mind. What would be the expected output for comparison filters when both key and value are undefined? If they are both undefined, do we consider them equal? In JS undefined === undefined is true. In Dust that somehow feels wrong.

{@eq key=undefined value=undefined}
   bar!
{:else}
   baz
{/eq}

Another thing to keep in mind is that fixing this would be somewhat backward incompatible. If template relied on nothing being output previously, it would require a code fix (to execute comparison filter only when key is defined). Relying on this behavior does not seem like a good practice though and I'm in favor of fixing this. We should do a minor version bump with this fix though.

@sethkinast
Copy link
Contributor

We should distinguish between cases where no key is specified (an error) and where key resolves to undefined (a possible outcome).

sethkinast pushed a commit to sethkinast/dustjs-helpers that referenced this issue Nov 18, 2014
…et but resolves to undefined.

Previously, if a helper's key parameter was set, but the Dust variable to which it pointed did not exist, the helper would immediately abort. However, this breaks cases especially for {@ne} in which it would be completely valid if the key did not exist.

The helper will still fail to execute if a key parameter is not passed. However, if the parameter resolves to an undefined variable, it will now execute.

This is a non-backwards-compatible change. Closes LinkedInAttic#59.
sethkinast pushed a commit to sethkinast/dustjs-helpers that referenced this issue Nov 18, 2014
…et but resolves to undefined.

Previously, if a helper's key parameter was set, but the Dust variable to which it pointed did not exist, the helper would immediately abort. However, this breaks cases especially for {@ne} in which it would be completely valid if the key did not exist.

The helper will still fail to execute if a key parameter is not passed. However, if the parameter resolves to an undefined variable, it will now execute.

This is a non-backwards-compatible change. Closes LinkedInAttic#59.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants