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

Use nsdeclare for namespace declarations #112

Merged
merged 3 commits into from
Aug 3, 2014
Merged

Conversation

lazd
Copy link
Contributor

@lazd lazd commented Jul 28, 2014

This PR uses the nsdeclare module for namespace declarations.

Changes:

  • Better output - nsdeclare avoids redeclaration of already declared namespace parts
  • DRY - use the same module as other Grunt plugins
  • Namespace declarations don't all happen at the top of the file, they happen inline
  • Consistently append Regex to cached regular expression variable names
  • Consistently use lowecase filepath to match filename
  • Put default values of options on a new line as intended in README

@lazd
Copy link
Contributor Author

lazd commented Jul 28, 2014

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
Copy link
Contributor Author

lazd commented Jul 28, 2014

@vladikoff
@sindresorhus

What do you think of my comment above?

@vladikoff
Copy link
Member

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

vladikoff added a commit that referenced this pull request Aug 3, 2014
Use nsdeclare for namespace declarations
@vladikoff vladikoff merged commit 37c469d into gruntjs:master Aug 3, 2014
@vladikoff
Copy link
Member

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

We have several issues regrading the namespaces:
#108
#69
#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
Copy link
Contributor Author

lazd commented Aug 3, 2014

@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?

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.

2 participants