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

Allow dust.helpers.tap on dust body functions #71

Merged
merged 1 commit into from
Feb 20, 2014

Conversation

kate2753
Copy link
Contributor

Calling dust.helpers.tap on dust body functions does not work because of linkedin/dustjs#423

This changes implementation of tap helper not to rely on isFunction flag set in dust core.

//if this function is dust body function, it will be rendered here with chunk.render call
//render result will be saved into output variable; render will return chunk object in this case
//if input is a simple function (not a dust body function) it will be invoked by chunk.render call;
//result of simple function execution will be returned and saved to out variable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you simplify the above comment since the chunk.render call is made regardless.

@kate2753
Copy link
Contributor Author

Updated comments and added additional unit test


//assume it's a simple function call when return result is not a chunk
//and re-assign out to output so it can be returned as final result
if(typeof out.untap === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!(out instanceOf Chunk) would be more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! I was looking for a way to do this. Will update the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use Chunk here :(

Sine helpers are in a different closure from dust core Chunk is not available here.

Is there any better way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if you did this:

if(Object.getPrototypeOf(out) !== Object.getPrototypeOf(chunk))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to MDN Object.getPrototypeOf is not supported by IE8 and earlier. Is this something we need to worry about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about out.constructor !== chunk.constructor ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works better

@jimmyhchan
Copy link
Contributor

sweet! this seems like a pretty complete solution. some side effects:

  1. before: context function are called with no arguments; with this change: they are now called with chunk, context... which seems better.
  2. isFunction is now ignored... passing a function with a property of isFunction of true will be ignored
  3. tapping a function that returns a chunk will give you the string rendering of the chunk instead of the chunk itself..

}

var dustBodyOutput = '',
simpleFunctionOutput;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about the name returnValue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returnValue instead of dustBodyOutput or simpleFunctionOutput ? I wanted to make names a bit more self explanatory. output and out I used initially seemed confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for simplefFunctionOutput. i think dustBodyOutput is nice as is.

definitely agree this is confusing. i think returnValue implies it's coming from a function...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like returnValue too, I think this name works well here. Updated to use returnValue instead of simpleFunctionOutput.

@prashn64
Copy link
Contributor

lgtm, Can you look at it once more and pull @jimmyhchan

jimmyhchan added a commit that referenced this pull request Feb 20, 2014
dustjs-linkedin#423 fix dust.helpers.tap to work with dust body functions. Fix tap helper to not rely on isFunction flag set in dust core. Using helper.tap on Context functions (function in your JSON context) will now get chunk and context as arguments.
@jimmyhchan jimmyhchan merged commit c383b44 into LinkedInAttic:master Feb 20, 2014
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 this pull request may close these issues.

3 participants