make tap helper work with function-valued context variables; add tests

[rodw]

This pull request addresses a problem with the tap function recently added to dust-helpers.js.

Tap doesn't always work properly when it encounters references to context keys that resolve to functions as opposed to scalar values.

For instance, given a base context object that includes a function:

    { foo: function() { return "bar"; } }

Dust will generally resolve a reference to foo by evaluating the function. I.e. the template {foo} will resolve to bar.

In some circumstances the tap function doesn't quite do that--yielding an error or a blank/undefined value.

Specifically, suppose we're creating a helper function named example that accepts an inline-parameter named value. We'd like to use the tap helper function (helper-helper-function?) to allow example to handle things like:

    {@example value="a literal string"/}

    {@example value=an_object_reference/}

    {@example value="a literal string containing {an_object_reference}"/}

    etc. 

in a dust-like way.But while tap works as expected for

    {@example value=an_object_reference/}

when an_object_reference resolves to a string, it gives an error with an_object_reference resolves to a JavaScript function.

The underlying cause for this is that the tap method uses typeof input === 'function' to identify the input values that need to be "rendered" with dust, but not every input function is a dust body-style function.

The change in this pull request is to check to see if input is a zero-argument function, in which case tap evaluates the function (i.e. output = input()) rather than trying to tap and render it as a dust template.

Unit tests for several tap cases are included in the patch.

[vybs]

Hey Rod !

Let me double check ! Thanks for the detailed explanation and the pr

./Vee

On Jul 16, 2012, at 11:59 AM, Rod [email protected] wrote:

This pull request addresses a problem with the tap function recently added to dust-helpers.js.

Tap doesn't always work properly when it encounters references to context keys that resolve to functions as opposed to scalar values.

For instance, given a base context object that includes a function:

   { foo: function() { return "bar"; } }

Dust will generally resolve a reference to foo by evaluating the function. I.e. the template {foo} will resolve to bar.

In some circumstances the tap function doesn't quite do that--yielding an error or a blank/undefined value.

Specifically, suppose we're creating a helper function named example that accepts an inline-parameter named value. We'd like to use the tap helper function (helper-helper-function?) to allow example to handle things like:

   {@example value="a literal string"/}

   {@example value=an_object_reference/}

   {@example value="a literal string containing {an_object_reference}"/}

   etc. 

in a dust-like way.But while tap works as expected for

   {@example value=an_object_reference/}

when an_object_reference resolves to a string, it gives an error with an_object_reference resolves to a JavaScript function.

The underlying cause for this is that the tap method uses typeof input === 'function' to identify the input values that need to be "rendered" with dust, but not every input function is a dust body-style function.

The change in this pull request is to check to see if input is a zero-argument function, in which case tap evaluates the function (i.e. output = input()) rather than trying to tap and render it as a dust template.

Unit tests for several tap cases are included in the patch.

You can merge this Pull Request by running:

git pull https://github.com/rodw/dustjs tap-helper-patch

Or you can view, comment on it, or merge it online at:

#93

-- Commit Summary --

• make @tap helper work with function-valued context variables; add tests

-- File Changes --

M lib/dust-helpers.js (38)
M test/core.js (183)

-- Patch Links --

https://github.com/linkedin/dustjs/pull/93.patch
https://github.com/linkedin/dustjs/pull/93.diff

---

Reply to this email directly or view it on GitHub:
#93

[vybs]

we use jasmie rather than the old test suite?

Do you mind updating the jas mine tests

Will merge the changes soon

[vybs]

any update on tests will be appreciated. thanks

[jairodemorais]

I am going to add the jasmine-test

[rodw]

Sorry everyone. I've had this on my todo list for a while but I haven't yet had a chance to dig into the jasmine-test part of it. (If no one else jumps on it first, can someone point me to specifically where in the current repo this test should go?)

For what it's worth, while it was news to me as well, it seems that the function.length property is a standard and pretty much universally supported JavaScript feature:

• It's been part of the ECMAScript standard at least since version 1.1 (covered in Section 15.3.5.1 of the latest version, available here).
• It's supported in all versions of Internet Explorer since IE6,
• in Mozilla/Rhino/Firefox,
• and as demonstrated by this test case, in Chrome/v8/Node.
• I couldn't find direct documentation of Opera and Safari (Nitro) support, but according to section 2.14 of the PDF document linked from here both seem to have had support for function.length for at least five years.

Ideally we'd literally only render the "dust functions" in this method, and against that standard this pull request is still a tiny bit of a hack. There could be non-dust functions that expect one or more arguments. But this patch brings us a lot closer to the objective. Since dust doesn't have a way to pass an argument to the "context functions", the vast majority of them probably aren't expecting one anyway.

A more definitive way to do this would be to somehow unambiguously mark the bodyxx functions that dust generates--maybe with a special property or prototype--but that seemed to me to be a lot more obtrusive and likely to have unexpected side-effects. (Alternatively, if the dust-generated functions had a very obscure naming convention we might be able to rely on that, but then we're replacing a dependency on function.length with a dependency on function.name or some kind of ugly hack based on function.toString neither of which seems as reliable as .length.)

[jairodemorais]

@rodw first of all, thanks for your time. Second I have migrated your test to jasmine, you can take a look here: LinkedInAttic/dustjs-helpers@5f59b59.

Regarding to the check, we are discussing how to resolve it.

[jairodemorais]

@rodw I have sent a new PR #105, with your changes and some other changes to recognize context functions in the tap method.