search

gruntjs / grunt-contrib-handlebars

Precompile Handlebars templates to JST file.
http://gruntjs.com/
MIT License
282 stars 126 forks source link

options.amd can be an array of dependencies instead of just a boolean or string #104

Closed nicjansma closed 10 years ago

nicjansma commented 10 years ago

This changes options.amd so it can be an Array instead of just a boolean or string.

This PR is similar to https://github.com/gruntjs/grunt-contrib-handlebars/pull/78, except it just uses the amd option and doesn't require an additional amdRequire option.

vladikoff commented 10 years ago

Thanks for the PR. Do you feel like the whole // ensure 'handlebars' is the first dependency checking is necessary? Can we just explain in the readme to do that and keep the task simpler? Let me know your thoughts!

nicjansma commented 10 years ago

We need to make sure we specify 'handlebars' first in the define([...]) array, since the first one is the one we expose as Handlebars in via the callback function:

define([" + amdString + "], function(Handlebars) { ...

But yes, it would be simpler if we just specified that requirement in the docs. It's up to you, I can change the PR if you don't want that complexity added.

vladikoff commented 10 years ago

I personally would love to have it in the documentation instead. The other thing: is there a need to expose other modules, i.e function(Handlebars, Module1, Module2) {? or is this only for Handlebars helpers?

nicjansma commented 10 years ago

PR updated (docs updated, logic in task removed).

In my case, with the handlebars.helpers module I use, no, there is no need to expose that module. The helpers simply extend the Handlebars object and aren't used within the templates module directly.

The difficulty I see with trying to expose those other modules is as with your example, what do we name them such that they're easy to use? IMO, exposing the other modules isn't needed, and we could wait for a use case before worrying about it.