Fixed :else in section when iterating over empty array. #127

zzen · 2012-08-28T18:20:55Z

The "No friends :(" basic example from dust documentation was actually not working. Is anybody using this stuff?

The "No friends" 101 example from dust documentation was actually not working

travisbot · 2012-08-28T18:21:34Z

This pull request passes (merged de0ce85 into 4b3fb75).

jairodemorais · 2012-08-28T18:28:09Z

@vybs we have discussed this in another issue, I could not find it. do u remember?

zzen · 2012-08-28T18:54:53Z

@jairodemorais - not sure what there is to discuss. It clearly breakes this verbatim sentence from documentation:

If, however, friends is an empty array or does not exist, the {:else} block will be executed and output "You have no friends :(".

Either this needs to be merged or the documentation needs to be changed.

vybs · 2012-08-28T18:59:54Z

@zzen easy !

If there is anything wrong in documentation lets correct it.

http://blogs.hbr.org/taylor/2012/08/its_more_important_to_be_kind.html

jairodemorais · 2012-08-28T19:18:42Z

@zzen please read this issue:

#79

After that we can talk.

zzen · 2012-08-28T20:03:14Z

@vybs - nothing wrong with the documentation in my view, hence the pull-request to fix the implementation.

@jairodemorais - I've read the issue, thanks for the link. In my view, fixing the documentation (and keeping the existing, broken behavior) is the wrong way to fix the problem.

I'd love to hear the reasoning behind why current status-quo is more reasonable. I don't think "there is a workaround with more convoluted syntax" is a good reason.

jimmyhchan · 2012-08-28T20:20:37Z

Not a great argument but empty arrays are truthy in js

zzen · 2012-08-28T21:39:19Z

@jimmyhchan - ok, that's a reasonable way to look at it. However if dust aligned itself with how JS evaluates stuff, then (?foo} for foo=0 would evaluate as false, which it unfortunately doesn't at this point... So clearly compatibility with true/false evaluations in JS is not the design intent.

In my view, if you're writing a template to iterate over all items in an array, then providing alternative content for empty array is the most typical job, most intuitive behavior - and the else fork is an elegant way to solve it.

The fact that this is the way it has been documented not only in the LinkedIn fork but also in the original @akdubya docs - speaks volumes about it being intuitive behavior. I'm just puzzled that nobody has not noticed all that time that the implementation doesn't actually match that behavior. Is it years since @akdubya wrote it?

vybs · 2012-09-04T04:11:50Z

Ok after some digging deeper ( updated the wiki )

There are two other special section notations that provide conditional testing:

{?name} not only tests the existence of name in the current context, but also evaluates it
{^name} not only tests the non-existence of name in the current context, but also evaluates it

Note that if "name" does exist, it is evaluated as follows (e.g "" or ' ' will evaluate to false, false will evaluate to false of course, but 0 evals to true, so does, "0", "false" will evaluate to true). Also note that empty [] is evaluated to false and empty {} is evaluated to true the ? tag. Since both pass the "exists" check and are empty, it should have been treated the same way.

Next, lets look at how the section tag works with exists and truthy/falsy evaluation

Template:

<ul>
{#friends}
    <li>{name}, {age}{~n}</li>
{:else}
    <p>You have no friends!</p>
{/friends}
</ul>

JSON:

   {
    friends: [
        { name: "Moe", age: 37 },
        { name: "Larry", age: 39 },
        { name: "Curly", age: 35 }
    ]
   }

This renders html as expected:

 <ul>
  <li>Moe, 37</li>
  <li>Larry, 39</li>
  <li>Curly, 35</li>
 </ul>

However if we change the friends array to be empty,

   {
    friends: [ ]
   }

the {:else} block does not trigger, we are instead left with an empty "ul" element.

You can achieve the desired behaviour by doing this,

     {?friends}
      <ul>
        {#friends}
          <li>{name}, {age}{~n}</li>
        {/friends}
      </ul>
     {:else}
      <p>You have no friends!</p>
     {/friends}

But this is really odd. The {#} does not behave the same as the {?}.

{?} treats [] as falsy and executes the else block. Why?

{#} treats the [] as exists, but does not execute either the section body nor the else block ( A BUG )

We need to fix this : atleast to be consistent between ? and # and then IMO # not executing either blocks is a real bug agreed

vybs · 2012-09-04T04:20:27Z

test/jasmine-test/spec/grammarTests.js

@@ -128,6 +128,13 @@ var grammarTests = [
    message:"should test the else block"
  },
  {
+    name:   "empty_else_block",
+    source: "{#foo}full foo{:else}empty foo{/foo}",
+    context: { foo: []},


can we add a few more tests for completeness

to make sure both ? and # behave the same way

0

"0"

true

false

[]

{}

We should fix the problem more upstream where dust seems to be treating numbers as string literals...i.e. 0 doesn't evaluate to false, which leads me to believe its treating it as "0". {} should evaluate to true in keeping with standard javascript.

for completeness then we should also check null and empty string

vybs · 2012-09-04T17:43:19Z

@jimmyhchan, @jairodemorais your thoughts?

jairodemorais · 2012-09-04T17:59:24Z

@vybs you did a great interpretation of the dust code, I think u are right, let's merge it after adding more tests!

vybs · 2012-09-05T20:18:33Z

lib/dust.js

      }
-      context.stack.head['$idx'] = undefined;
-      context.stack.head['$len'] = undefined;
-      return chunk;
    }
  } else if (elem === true) {
    if (body) return body(this, context);


Why not do it by using the dust.isEmpty instead of len>0 that the exists section uses

dust.isEmpty = function(value) {
if (dust.isArray(value) && !value.length) return true;
if (value === 0) return false;
return (!value);

--len is needed in the loop anyway. no need to call another function.--
nevermind. totally agree let's keep consistent and use isEmpty.. let's put it before the isArray check so that is checked first and for everything type.

jimmyhchan · 2012-09-06T01:03:13Z

FWIW the other clientside templating languages have this issue as well. Much debated. There seems to be some argument between what is correct and what is usable.

The difference between {?foo} and {#foo} is useful
mustache/mustache#47

handlebar added an each and an if helper to mustache that will render checks false, undefined, null or [ ] ... { } seems to be treated as falsey as well.

It seems we need to be very careful what we pick.

vybs · 2012-09-06T04:59:50Z

@jimmyhchan on other note, I am curious how they do the if helper:)

jimmyhchan · 2012-09-06T07:05:56Z

Ok i think i steered the conversation the wrong way.

whatever boolean check we end up using, falsy sections should execute the else block... which this PR addresses.
currently #section and ?exists are identical except the empty array difference (and the context change)

+1 on @vybs suggestion to use isEmpty.
Let's move the else if (skip) block up as the first check before the isArray. This will be a lot cleaner. we could potentially even remove the elem === 0 check in that last if

jimmyhchan · 2012-09-06T07:16:22Z

So reading through the plethora of bugs regarding what the correct truthy definitions should be, I think Dust does a good job in picking truthy values that make sense in templates.

As pointed out earlier, everything in truthy except:
null, false, [ ] , ' ', and undefined

when you refer to a key that doesn't exists... it is falsy because the value is undefined.
when you refer to a key whose value is { } ... it is truthy

vybs · 2012-09-06T07:21:05Z

we should update this in the wiki.

except the 0 ( is treated as true )

BTW, is this line of code ever executed in the dust.isEmpty.

if (value === 0) return false;

I could not see it covered int he code coverage. May be we dont have enough tests

jimmyhchan · 2012-09-06T16:00:05Z

there is a zero check in the tests and asserts that 0 is truthy that is... !(isEmpty(0))

vybs · 2012-09-06T16:02:25Z

unless i am missing something, the code says

if (value === 0) return false;

vybs · 2012-09-10T07:09:28Z

Merging this PR, since we all agree we need this.

Fixed :else in section when iterating over empty array.

GH-85, GH-127 improve test coverage

billinghamj · 2012-10-16T21:32:33Z

So to confirm... the following:

[{"key":0}]

{?.key}
a
{:else}
b
{/.key}

should output b? I'm not getting that result personally... it's returning a for me.

vybs · 2012-10-16T22:33:49Z

I tried the same on this tool: http://linkedin.github.com/dustjs/test/test.html

I get b

billinghamj · 2012-10-16T22:41:38Z

Okay, I'll carefully test and check before letting you know where I get. :)

vybs · 2012-10-16T22:44:19Z

np ! does the tool output a for you>

billinghamj · 2012-10-16T22:55:39Z

The library (not in the tool) appears to output the equivalent of "a", but what I'm actually working with is a bit more complex. Just going to work through a few test cases on my own stuff to try and either identify that I'm not having the problem or to narrow down the cause a little more. :)

billinghamj · 2012-10-16T23:13:09Z

Okay. I'm pretty certain it's a problem. I've stripped down my code a lot to try to make it understandable.

Using the tool with this for the template:

{#cards}
        {?.stars}
          <div class="ratings r{.stars}"><span>{.stars} stars,</span> {.ratings} ratings</div>
        {:else}
          <div class="ratings r0"><span>no stars,</span> not enough ratings</div>
        {/.stars}
        {~n}
{/cards}

And this for the JSON:

{cards: [
    {},
    { stars: 5, ratings: 5000 },
    { stars: 4, ratings: 4000 },
    { stars: 3, ratings: 3000 },
    { stars: 2, ratings: 2000 },
    { stars: 1, ratings: 1000 },
    { stars: 0, ratings: 10 },
    { ratings: 10 },
]}

Both the last & second to last should output the same (correct?). But the second to last outputs "0 stars" (wrong) and the last outputs "no stars" which is correct.

Maybe I'm missing something... not sure.

Output I'm getting:

<div class="ratings r0"><span>no stars,</span> not enough ratings</div>
<div class="ratings r5"><span>5 stars,</span> 5000 ratings</div>
<div class="ratings r4"><span>4 stars,</span> 4000 ratings</div>
<div class="ratings r3"><span>3 stars,</span> 3000 ratings</div>
<div class="ratings r2"><span>2 stars,</span> 2000 ratings</div>
<div class="ratings r1"><span>1 stars,</span> 1000 ratings</div>
<div class="ratings r0"><span>0 stars,</span> 10 ratings</div>
<div class="ratings r0"><span>no stars,</span> not enough ratings</div>

billinghamj · 2012-10-16T23:16:04Z

In fact, I'm even getting "a" with the basic example: http://puu.sh/1fBc9

Using Safari on OS X btw.

vybs · 2012-10-17T02:30:54Z

So the .key and .stars seem to be cause, its easy to see the exists section code in the dust.js and see how it evaluates them

vybs · 2012-10-17T03:31:42Z

Kind of behaved the same in : http://akdubya.github.com/dustjs/

billinghamj · 2012-10-17T15:18:46Z

So are we confirming that this is an issue, or not? Both interpreters are giving me the same response.

vybs · 2012-10-17T15:22:08Z

Please file a ticket on github, since this is closed.

Fixed :else in section when iterating over empty array.

de0ce85

The "No friends" 101 example from dust documentation was actually not working

vybs reviewed Sep 4, 2012
View reviewed changes

vybs reviewed Sep 5, 2012
View reviewed changes

vybs added a commit that referenced this pull request Sep 10, 2012

Merge pull request #127 from zzen/patch-2

00d3d2d

Fixed :else in section when iterating over empty array.

vybs merged commit 00d3d2d into linkedin:master Sep 10, 2012

vybs added a commit that referenced this pull request Sep 10, 2012

Merge pull request #139 from linkedin/vybs-dustjs

b812dc3

GH-85, GH-127 improve test coverage

billinghamj mentioned this pull request Oct 17, 2012

"falsey" conditional {?.x} is not treating 0 as false #180

Closed

jimmyhchan mentioned this pull request Jun 11, 2013

exists / isEmpty with functions #282

Closed

Fixed :else in section when iterating over empty array. #127

Fixed :else in section when iterating over empty array. #127

Conversation

zzen commented Aug 28, 2012

travisbot commented Aug 28, 2012

jairodemorais commented Aug 28, 2012

zzen commented Aug 28, 2012

vybs commented Aug 28, 2012

jairodemorais commented Aug 28, 2012

zzen commented Aug 28, 2012

jimmyhchan commented Aug 28, 2012

zzen commented Aug 28, 2012

vybs commented Sep 4, 2012

vybs Sep 4, 2012

Choose a reason for hiding this comment

jleppert Sep 5, 2012

Choose a reason for hiding this comment

jimmyhchan Sep 6, 2012

Choose a reason for hiding this comment

vybs commented Sep 4, 2012

jairodemorais commented Sep 4, 2012

vybs Sep 5, 2012

Choose a reason for hiding this comment

jimmyhchan Sep 6, 2012

Choose a reason for hiding this comment

jimmyhchan commented Sep 6, 2012

vybs commented Sep 6, 2012

jimmyhchan commented Sep 6, 2012

jimmyhchan commented Sep 6, 2012

vybs commented Sep 6, 2012

jimmyhchan commented Sep 6, 2012

vybs commented Sep 6, 2012

vybs commented Sep 10, 2012

billinghamj commented Oct 16, 2012

vybs commented Oct 16, 2012

billinghamj commented Oct 16, 2012

vybs commented Oct 16, 2012

billinghamj commented Oct 16, 2012

billinghamj commented Oct 16, 2012

billinghamj commented Oct 16, 2012

vybs commented Oct 17, 2012

vybs commented Oct 17, 2012

billinghamj commented Oct 17, 2012

vybs commented Oct 17, 2012