gruntjs / grunt-contrib-handlebars

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

Use nsdeclare for namespace declarations #112

Closed lazd closed 10 years ago

lazd commented 10 years ago

This PR uses the nsdeclare module for namespace declarations.

Changes:

lazd commented 10 years ago

So there's one thing this PR is missing. You can let nsdeclare do the assignment for you, in which case you avoid manually generating the code for the assignment and using JSON.stringify to escape the property. I started off doing this, but then realized the output wouldn't match the tests. Here's why:

By default, for a options.namespace of JST and a filename of templates/myfile.hbs grunt-contrib-handlebars will compile that template to this["JST"]["templates/myfile.hbs"]. This makes a lot of sense.

When using nsdeclare to do the assignment, the template will be compiled to this["JST"]["templates/myfile"]["hbs"], which doesn't make much sense at first glance...

However, assume grunt-contrib-handlebars included a default options.processName that removes the filename's extension, then this becomes insanely powerful out of the box as templates/Modal.base.hbs get compiled to this["JST"]["Modal"]["base"] with no additional configuration.

To achieve this with grunt-contrib-handlebars as-is, you would need to use options.namespace as a function as well as a options.processName that returns the last part of the filename before the extension.

In grunt-domly, I use nsdeclare's value option to perform the assignment and a default processName that removes the file extension to get the "automatic sub-namespaces based on filename" out of the box, and it's super useful.

Obviously, we can't do this by default in grunt-contrib-handlebars as it would break tons of people's builds, but we could definitely provide options to allow this. That's another PR, however. Thoughts?

lazd commented 10 years ago

@vladikoff @sindresorhus

What do you think of my comment above?

vladikoff commented 10 years ago

@lazd thanks for this. Will look into it as soon as I can

vladikoff commented 10 years ago

@lazd Excellent, thanks for providing sufficient comments as well.

We have several issues regrading the namespaces: https://github.com/gruntjs/grunt-contrib-handlebars/issues/108 https://github.com/gruntjs/grunt-contrib-handlebars/issues/69 https://github.com/gruntjs/grunt-contrib-handlebars/pull/96 etc.

as it would break tons of people's builds,

We can ship breaking changes in 0.9.0, etc. @lazd If you want to PR the improvements you suggested that will be appreciated by many. Also +1 to modifying the current default processName

lazd commented 10 years ago

@vladikoff, thanks! I added some comments to the issues referenced, I can definitely send a PR that handles these issues as well as adds the functionality I proposed in my comment above.

What is the timeline for a 0.9.0 release?