adammark / Markup.js

Powerful JavaScript templates
317 stars 51 forks source link

Potential gotcha with chaining Boolean pipes with 0 as input value #13

Open mklement0 opened 11 years ago

mklement0 commented 11 years ago

Note sure there's a solution to this, but it's at least worth pointing out in the read-me as a potential gotcha:

Due to Boolean pipes passing through their input values in case of successful test, comparing something to zero will yield unexpected results, if:

Two examples:

{{levelIndex|equals>0|choose>ZERO>NOT_ZERO}} # Unexpectedly prints 'NOT_ZERO' if levelIndex is 0. {{levelIndex|less>1|choose>ZERO>NOT_ZERO}} # Unexpectedly prints 'NOT_ZERO' if levelIndex is 0.

adammark commented 11 years ago

Ah, good catch! This is a problem with the "choose" pipe:

    choose: function (bool, iffy, elsy) {
        return !!bool ? iffy : (elsy || "");
    },

where !!0 returns false.

Should be a quick fix.

adammark commented 11 years ago

On second thought, let me think about whether it makes sense to change this. I might just change the docs to say that the "choose" pipes evaluates truthy/falsy instead of true/false. (0 is falsy, but -1 is not.)

mklement0 commented 11 years ago

Doesn't the problem lie with the passing-the-input-value-through-conceptually-Boolean-pipes-if-true behavior rather than with the choose pipe?

I agree that changing the choose pipe is not the way to go.

Short of creating strictly-Boolean variants of existing pipes - ones that return actual true and false rather than their input - I don't think there's much you can do without potentially breaking existing code.

If you do change the docs, I suggest you explain that the conceptually-Boolean pipes yield unexpected results with falsy input when their output is passed to a truthy/false-input pipe such as choose, and that using an {{if}} statement is a workaround.

Conceivably, you could create a single new pipe to address the issue; e.g. :

Mark.pipes.asBool = function (input) {
    return input === false ? false : true;
}

When placed after a conceptually-Boolean pipe (assuming its input is non-Boolean), it converts that pipe's output to a true Boolean that could then be passed to pipes such as choose.

The original example would then morph into:

{{levelIndex|equals>0|asBool|choose>ZERO>NOT_ZERO}}

Not great, but at least it's a simple solution that allows one to use the pithiness of the choose pipe even with falsy input to the preceding conceptually-Boolean pipe.

The name asBool is just an example - tough to find a name that's both descriptive and short, esp. given that you already have a bool pipe that acts on truthy/falsy.

adammark commented 11 years ago

Makes sense, but since choose is the only built-in pipe that evaluates a boolean input, it might be better to simply add an optional "strict" argument to this pipe alone:

{{levelIndex|equals>0|choose>ZERO>NOT_ZERO>strict}}

Thoughts?

mklement0 commented 11 years ago

Good idea.

Would be nice if the read-me mentioned:

In addition, and as a potential alternative, adding a built-in asBool pipe as discussed could be useful as well - that way, by inserting this pipe before a custom Boolean-input pipe, not every instance of the latter has to implement the strict argument.

On Dec 4, 2012, at 5:32 PM, Adam Mark notifications@github.com wrote:

Makes sense, but since choose is the only built-in pipe that evaluates a boolean input, it might be better to simply add an optional "strict" argument to this pipe alone:

{{levelIndex|equals>0|choose>ZERO>NOT_ZERO>strict}} Thoughts?

— Reply to this email directly or view it on GitHub.