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

Add function support to helpers #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jimf
Copy link

@jimf jimf commented Jun 26, 2015

Update formatDate, formatTime, formatRelative, formatNumber, and
formatMessage to allow for their primary argument to be specified as a
function that returns the required argument type.

Update `formatDate`, `formatTime`, `formatRelative`, `formatNumber`, and
`formatMessage` to allow for their primary argument to be specified as a
function that returns the required argument type.
@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@jimf
Copy link
Author

jimf commented Jun 26, 2015

CLA signed. Should I close this PR and open another?

@caridy
Copy link
Collaborator

caridy commented Jun 26, 2015

I imagine that the function is just a another helper, why not just use a handlebars' subexpression?

{{formatNumber (numFn)}}

in which case you have full control over the values to be passed to that helper, etc.

@jimf
Copy link
Author

jimf commented Jun 28, 2015

It's not, at least not formally. It's for supporting model presenters, identical to what's laid out in pragmatic-backbone.

This pattern is supported by all of the built-in Handlebars helpers. Example: https://github.com/wycats/handlebars.js/blob/93faffa549166c492267cc96d3e6848923760d90/lib/handlebars/base.js#L175

@caridy
Copy link
Collaborator

caridy commented Jun 29, 2015

I will like @ericf to chime in. I really don't like that pattern, just like I don't like these helpers to support a path to resolve a message, etc.

As a side note: those check for function type are not enough, you need something better to truly detect functions.

@caridy
Copy link
Collaborator

caridy commented Jun 29, 2015

It's not, at least not formally. It's for supporting model presenters, identical to what's laid out in pragmatic-backbone.

I don't fully understand what do you mean here. Can you explain why subexpressions will not work just fine?

@jimf
Copy link
Author

jimf commented Jun 29, 2015

As a side note: those check for function type are not enough, you need something better to truly detect functions.

No problem. Handlebars exposes a Handlebars.Utils.isFunction util. I'd be happy to switch over to that if preferred.

I don't fully understand what do you mean here. Can you explain why subexpressions will not work just fine?

Sub-expressions could work, assuming I were to write a Handlebars helper to effectively call the function and return the result. The workflow I'm describing however involves sending JavaScript class instances (aka Model Presenters) as the context to the Handlebars template (for sufficiently complex views; the idea being to keep the complexity out of Handlebars and in more easily tested code) as opposed to plain objects with static properties. These classes generally expose view-centric logic on top of their corresponding models in the form of functions. In the case of handlebars-intl, this simply requires statically assigning values to this in the presenter constructor, rather than exposing a function on the prototype like we do nearly everywhere else where this pattern is used (hopefully this made sense. If it'd help, I can put a gist/fiddle together). This is totally acceptable. It'd just be cool if these helpers worked like the base ones do so there's no special treatment.

@jimf
Copy link
Author

jimf commented Jun 29, 2015

Update: I looked into subexpressions more, since truthfully I didn't know much about them. The Handlebars expression docs lead me to believe that sub-expressions evaluated as Handlebars helpers, which I'm not working with. However, to my surprise while testing out your suggestion, it turned out to actually work. I'm digging around in the Handlebars source to figure out how that all works, and I don't have my head wrapped around it fully yet, but this testcase certainly confirms that it's expected functionality. So take that for what you will. I do still like the idea of the handlebars-intl helpers functioning the same way as the built-ins, but I understand if you'd rather keep them pure in terms of expected input.

Just playing devil's advocate with myself, I can't actually find anywhere in the Handlebars docs that explicitly states that Handlebars will evaluate and return the return values of functions that are passed with the context. But that said, it's clearly supported, as well as relied upon, for even that testcase above to work.

@paulfalgout
Copy link

paulfalgout commented Jul 19, 2017

A significant difference between the handlebars sub-expressions and how it would simply call the function is scope. handlebars data functions can call other data functions, but using sub-expression loses the scope can lose scope. However this PR only partially solves this issue and doesn't address passing data to formatMessage

Edit: I think the pain point here at least for my use case is switching from some template string containing {{ aFunctionData }} to {{formatMessage (intlGet "some.template.string") aFunctionData=aFunctionData}} isn't going to work.. particularly if the function uses its scope. But turns out this'd be true for any helper in handlebars and sub-expression is likely the best solution for these situations.

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.

4 participants