Making AMD module output return compiled function when only one template is processed #377

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@JamesMaroney
Contributor

Using Handlebars in an AMD environment, it would be helpful to return the compiled function from the AMD module.

To minimize impact on your current logic, I have only implemented this modification for when one template is being precompiled.

JamesMaroney added some commits Nov 27, 2012
@JamesMaroney JamesMaroney Update bin/handlebars
Making amd precompilation return the compiled function from the created module
ea74e39
@JamesMaroney JamesMaroney Update bin/handlebars
modifying added return statement to only apply when a single template
is being compiled
029da2e
Collaborator

This seems like something that should be an explicit opt in rather than an implicit behavior based on the number of templates passed in. This could impact users who are making multiple calls to the binary but not expecting a return.

Collaborator
kpdecker commented Apr 6, 2013

Closing this as I don't think this is the right direction. Will gladly accept a pull request that does this via a flagged parameter!

@kpdecker kpdecker closed this Apr 6, 2013
Contributor

I can certainly look into putting in an option to trigger this functionality. It really seems so strange to have this stance though. If users are using this as an AMD module, then they should be expecting a return value from the module. That's just how AMD works.

Do you have a preferred option name, or is "returnTemplateFromAmdModule" correct? Of course, then I'll need to add more code to verify that this is only used in just the right scenario, and documentation on just when to use it.

As I type this, I really think the correct approach here is to remove the conditionality on compiling only one template, and decide what gets returned from the AMD module when compiling multiple templates. Giving the users options they don't care about usually makes your product seem overly complicated, rather than flexible.

I would love if we could solve this, as I am currently working off of a fork of handlebars.js and I'd love to come back to the mainline.

Collaborator
kpdecker commented Apr 6, 2013

My issue here is that you are assuming that someone who is only using one template is using AMD which to me is an odd assumption to make. I.e. this will break if someone is not using amd or is otherwise concatenating the output with other code.

So we currently have the -a option for generating amd output. This currently does not return anything and as you mentioned the question is what should we return.

What if we always return the templates object from amd methods and add a special case to the namespace argument "none" or similar that will cause the amd methods to create a new namespace for each function instance?

i.e.

handlebars -a --namespace none $files

Would generate something like

define(['handlebars'], function(Handlebars) {
  var templates = {};
  template['foo'] = foo;
  template['bar'] = bar;
  return templates;
});

I'm not an AMD user so I don't know how well that works in practice.

We are hoping to cut a release soon so we might be able to get something out on npm quite quickly if we can get this figured out in time.

Contributor

ah, yes, I see. It wasn't my intent to affect any behavior in the non-AMD path. This was an oversight on my part for sure. Now I much better understand your hesitation :)

I'll need to take a few minutes to understand your suggestion. In the meantime, let me tell you a little bit about how we've come to use handlebars with AMD, and it might help you think of the proper way to tackle the problem.

Initially, I had written a little shim to make handlebars.js act as a requirejs plugin so that we could do this:

define(['handlebars!path/to/template.hb'], function(myTemplate) {
  // myTemplate now being the compiled template function
});

The commit that this pull request is covering is to remove the need for the handlebars! component and instead pre-compile the templates - but keep the modularity. With this patch, now we do this instead:

define(['path/to/template.hb'], function(myTemplate) {
  // myTemplate now being the pre-compiled template function
});

While we could handle the return value being the full templates object, it would add verbosity to our only usage. It would, however, be a single, standard solution. Selfishly, I'd love it if it would return just the function if it compiled only one, or the templates object if it compiled multiple. Would that work for you?

Collaborator
kpdecker commented Apr 6, 2013

There isn't any existing behavior here to worry about breaking WRT return values. The question is if it's odd to have different return value for one case vs. the other. I think it's fine if the single case returns the template directly.

Contributor

ok, cool. By when should I get the pull request updated in order to get this in the next release?

Collaborator
kpdecker commented Apr 6, 2013

The plan right now is to try to roll the release tomorrow or early next
week sometime.

On Sat, Apr 6, 2013 at 6:07 PM, James Maroney notifications@github.comwrote:

ok, cool. By when should I get the pull request updated in order to get
this in the next release?


Reply to this email directly or view it on GitHubhttps://github.com/wycats/handlebars.js/pull/377#issuecomment-16005809
.

Contributor

ok. I think I can get some time tonight to get this in. Thanks for all the feedback. My team will be happy to come back to the mainline handlebars repo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment