Skip to content
This repository has been archived by the owner on Mar 26, 2018. It is now read-only.

adding grunt-angular-templates to the build tasks #277

Closed
wants to merge 1 commit into from
Closed

adding grunt-angular-templates to the build tasks #277

wants to merge 1 commit into from

Conversation

vkammerer
Copy link

Hello everyone,

The package 'grunt-angular-templates' could be a great task to add to the build process.
It concatenates all html templates and registers them in the templateCache, which proves to be a massive performance improvement (no more need to request html partials when needed).

More info here:

I am not very confident about my implementation, but it is working fine on my end, so I still open the PR.

@passy
Copy link
Member

passy commented Jul 12, 2013

Thanks for the PR, @vkammerer. Personally, I have no experience with template precompilation and Angular, but it looks interesting.

@btford Any thoughts on this?

@eddiemonge
Copy link
Member

how does this play with usemin and htmlmin?

@btford
Copy link
Contributor

btford commented Jul 17, 2013

Thanks for the PR!

I'd like to see your grunt task more thoroughly tested before I add it to this generator. I'd especially like to see tests for invalid templates (missing closing tags, malformed urls, etc.) and edge cases.

@ericclemmons
Copy link

I'll chime in a bit here. There are sizeable performance improvements with minifying the HTML prior to concatentation, so I'll be adding an option to do this inline. (JSMin won't minify string literals, so there a sizeable savings)

As for the "invalid templates" aspect, I think there's legitimacy to some & others are truly edge cases. For example, I believe an invalid template path will throw the usual ding from grunt, since the file can't be found. Missing closing tags & the like sounds like completely optional html validation that should probably be done prior to the task running, much like running jshint before concatenating JS.

@btford
Copy link
Contributor

btford commented Jul 19, 2013

that should probably be done prior to the task running

Sure. I just want to ensure that stringifying and inlining the HTML doesn't sacrifice debug-ability. Do you know of a task that does this? #lazyweb

@vkammerer
Copy link
Author

@btford : I see that @ericclemmons has implemented htmlmin support to his package (see ericclemmons/grunt-angular-templates#32).
Does that mean we can go further with the integration of grunt-angular-templates?

@ericclemmons
Copy link

Are you guys stalking me again!?

@sindresorhus
Copy link
Member

merge conflict needs to be resolved.

@@ -18,6 +18,7 @@ module.exports = function (grunt) {

// configurable paths
var yeomanConfig = {
name: require('./bower.json').name + 'App',
Copy link
Member

Choose a reason for hiding this comment

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

this might not always be the case now. Instead of app, app-suffix should be used as a template string so when the gruntfile is generated, its using the correct suffix

Copy link
Author

Choose a reason for hiding this comment

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

I am in the middle of the rebase, but I don't know how to implement the app-suffix option. @eddiemonge Can you show me how to properly define the app name here?

@eddiemonge
Copy link
Member

i think this issue would need to be resolved first: ericclemmons/grunt-angular-templates#44

@ghost ghost assigned eddiemonge Nov 19, 2013
@eddiemonge
Copy link
Member

definitely want to pull this in but need to rebase first. Ive been playing with the grunt plugin and it does seem good but there are some consistency issues with it that im working out. if i can get those sorted i would whole heartedly recommend landing a modified version of this PR

@vkammerer
Copy link
Author

@eddiemonge good idea, the PR should maybe be redone altogether, don't you think?

Also, I don't know why commit https://github.com/vkammerer/generator-angular/commit/c65b92eaf8109bdea412896f9de2c2ea15dd9b77 was merged with it as it isn't related.

@eddiemonge
Copy link
Member

@vkammerer you most likely did a pull merge instead of a rebased pull.

@vkammerer
Copy link
Author

@eddiemonge : @btford added the commit.
@btford is there a reason why https://github.com/vkammerer/generator-angular/commit/c65b92eaf8109bdea412896f9de2c2ea15dd9b77 was added to this PR ?

@eddiemonge
Copy link
Member

@vkammerer yeah all you have to do is git pull --rebase upstream grunt-angular-templates and then resolve the merge conflicts and then push those changes up git push -f origin grunt-angular-templates and those commits will go away.

@marcalj
Copy link

marcalj commented Nov 25, 2013

Hi, I can't make it to work. As you can see in this issue:
https://github.com/amayvs/TestApp/issues/1#issuecomment-29171045

I want to achieve this:


In development I don't care about templates.js I'm ok loading each template independently. As I'm not using grunt watch in my workflow it's easier for me.

In production I want a minified views in JS concatenated in scripts.js with all images revisionated. Just with grunt build taking care of present state.


In https://github.com/amayvs/TestApp assumes that templates.js have to exist, as he adds a watch task for it.
The main problem is with assets (images) that don't get renamed with revisioned ones.

I think this should be managed with usemin process, but I was unable to make it work, neither with your branch. Are you using new template system? Are this '<%%= yeoman.app %>' a bug? (instead of '<%= yeoman.app %>').

@eddiemonge if you have a test app with this process, please let me know. And if you prefer to treat this issue in your repo, no problem, just let me know.

Thanks!

@ericclemmons
Copy link

Just released v0.5.0 & closed ericclemmons/grunt-angular-templates#44, which means that having all of your templates inlined in your HTML during development & only running ngtemplates as part of deployment after useminPrepare should now work.

@vkammerer
Copy link
Author

@ericclemmons after upgrading to grunt-usemin@2.0.2 and grunt-angular-templates@0.5.0, I am still getting :

Concat target not found: public/scripts/scripts.js

@marcalj
Copy link

marcalj commented Dec 2, 2013

Mee too. Details on ericclemmons/grunt-angular-templates#44

@ericclemmons
Copy link

Well, crap, back to the pile!

@ericclemmons
Copy link

@marcalj Can you paste up your ngtemplates config? This is what I'm testing with:

https://github.com/ericclemmons/grunt-angular-templates/blob/master/Gruntfile.js#L92-L98

@ericclemmons
Copy link

Whoops, wrong thread! The target my tests are referencing are generated, not build:js's path/to/app.js. We'll work it out...

@eddiemonge
Copy link
Member

I've been testing this and trying to come up with some way to make it work. My progress somewhat is here: https://github.com/eddiemonge/generator-angular/commit/4cbf4c2673c60681f57a3e746bb00a05a790adc0

I've also been doing more work locally.

Before I can feel comfortable landing this, the last issue to work out is how to handle assets in the views that usemin wont get to update. I've been trying to have multiple task configs that will run usemin, ngtemplates, concat, and then uglify but Im stuck at rev's updated file names and how to make those targets.

I don't want to say assets can't be allowed in the views and to use images you will have to use css backgrounds with class names but unless someone can figure out how to make it all work with usemin, that might have to be the solution.

@ericclemmons thoughts?

@ericclemmons
Copy link

Howdy @eddiemonge! Take a look @ v0.4.10 (or v0.5.1, if you've already moved on). Regarding the rev thing, I can possibly research it, but I haven't used it at all yet, since I'm not a fan of single-page apps have non-server-rendered pages. Open an issue in the grunt-angular-templates project with a bit more detail & we can resolve it there.

Dunno if that's a show-stopper for this PR, though.

@marcalj
Copy link

marcalj commented Dec 16, 2013

@eddiemonge I think you're aware of this, but if not you can do it with patterns. We have to wait for usemin to close the issue:

yeoman/grunt-usemin#244 (comment)

@eddiemonge
Copy link
Member

That would require manually specifying all the assets though, I think. For this type of thing that isn't an ideal solution. I'll keep tinkering with it though.

@marcalj
Copy link

marcalj commented Dec 17, 2013

Yep, absolutelly not ideal solution, but perfect workaround for the meantime.

@louisremi
Copy link

I have a fairly ugly workaround that works fairly well too:

ngtemplates: {
  app: {
    cwd: '<%= yeoman.app %>',
    src: 'views/**/*.html',
    dest: '.tmp/templates/templates.js'
  }
}

And in my usemin block:

<!-- build:js scripts/scripts.js -->
<script src="scripts/app.js"></script>
<script src="../.tmp/templates/templates.js"></script>
<script src="scripts/controllers/main.js"></script>
<!-- endbuild -->

When using grunt serve, the pre-compiled templates will throw a 404 and html templates will be fetched instead. Using grunt build will result in pre-compiled templates to be concatenated, revved and uglified like any other script.

Yes, I know, it's ugly.

@eddiemonge
Copy link
Member

@vkammerer can you rebase this please. Also could you add a doc note about images/assets in the views probably won't get rev'ed?

@vkammerer
Copy link
Author

@eddiemonge I have just rebased and updated the PR. As you can see, I changed the configuration after looking at https://github.com/eddiemonge/generator-angular/commit/4cbf4c2673c60681f57a3e746bb00a05a790adc0 .

Also, I added a js pattern for usemin that takes care of the images rev'ing: https://github.com/vkammerer/generator-angular/blob/86f377c4be735afde7ccd204d3eb37525e6ad454/templates/common/Gruntfile.js#L269 . So far, it seems that the current regexp is restrictive enough, but any improvement suggestion to it would be very welcome.

@sindresorhus
Copy link
Member

@eddiemonge did you still want this? If so the merge conflict needs to be resolved ;)

@eddiemonge
Copy link
Member

I really am scared of this one. Waiting to see how yeoman/generator-webapp#371 turns out with the JS regex though.

@eddiemonge eddiemonge added this to the 0.10.0: The Future milestone Jun 5, 2014
@elegantcoder
Copy link

I'm waiting for this PR to be closed :-)

@geoah
Copy link

geoah commented Aug 13, 2014

👍

@eddiemonge
Copy link
Member

It wont be closed. It might get merged in a different form but its still getting in at some point

@vkammerer
Copy link
Author

@eddiemonge is there any change you would like me to implement for the PR to be merged?

@eddiemonge
Copy link
Member

nope. i was going to pull the changes into my rewrite. im trying to do as few changes as possible so i can devote more time to that.

@sindresorhus
Copy link
Member

@eddiemonge So what's the plan with this?

@icaliman
Copy link

icaliman commented Apr 2, 2015

Merge this please! It will fix issues when browser caches view templates.

@eddiemonge
Copy link
Member

:( that tests are broken now. gotta fix that for a new release

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.