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

added templateWrap callback option and strictMode boolean option #119

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

taylorcode
Copy link

I think that it is useful if the developer can decide how the templates are registered. This allows them to use a different caching service, or even generate a json file (for example) with the templates instead of a js file.

@philkeys
Copy link

This would be great please merge this!

@fitiskin
Copy link
Contributor

Any plan to accept/review this PR? Must have feature :)

@ericclemmons
Copy link
Owner

@taylorcode Sorry for the delay. I'm down for merging this, but would need the forked changes in your README.md and package.json reset ;)

@ericclemmons
Copy link
Owner

Can anyone here help with #137?

@@ -47,7 +48,7 @@ var Compiler = function(grunt, options, cwd) {
// Append formatted URL
path += Url.format( Url.parse( url.replace(/\\/g, '/') ) );

return "\n $templateCache.put('" + path + "',\n " + template + "\n );\n";
return options.templateWrap(path, template, index, files);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a reason why the template wrapper function might need the index or the full list of files? It doesn't look like they're used in your examples.

@underscorebrody
Copy link
Collaborator

@taylorcode taking a look at this, and I really like the feature. Just had one question, and would request one or more tests for it. Also would you mind backing out the fork-specific changes to the README and package.json? Thanks!

@underscorebrody
Copy link
Collaborator

This also needs rebase

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.

5 participants