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

New concat option to add to concat config automatically #17

Merged
merged 3 commits into from
May 6, 2013
Merged

New concat option to add to concat config automatically #17

merged 3 commits into from
May 6, 2013

Conversation

cgross
Copy link

@cgross cgross commented Mar 29, 2013

If you use a task like grunt-usemin to pull your concat config from your html file, then its not possible to just add a new line to your concat config. Instead you want this to happen automatically.

So this adds a new option for ngtemplates to automatically have the dest file added to an existing concat config.

@ericclemmons
Copy link
Owner

Let me look into this real quick. I'm using usemin and concat now with no problems.

Do you have a concat config setting at all, or is usemin creating that behind the scenes for you?

Eric Clemmons
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Friday, March 29, 2013 at 9:34 AM, Chris Gross wrote:

If you use a task like grunt-usemin to pull your concat config from your html file, then its not possible to just add a new line to your concat config. Instead you want this to happen automatically.
So this adds a new option for ngtemplates to automatically have the dest file added to an existing concat config.
You can merge this Pull Request by running
git pull https://github.com/cgross/grunt-angular-templates master
Or view, comment on, or merge it at:
#17
Commit Summary
New concat option to support automatic concat config addition

File Changes
M README.md (https://github.com/ericclemmons/grunt-angular-templates/pull/17/files#diff-0) (11)
M tasks/angular-templates.js (https://github.com/ericclemmons/grunt-angular-templates/pull/17/files#diff-1) (21)

Patch Links:
https://github.com/ericclemmons/grunt-angular-templates/pull/17.patch
https://github.com/ericclemmons/grunt-angular-templates/pull/17.diff

@cgross
Copy link
Author

cgross commented Mar 29, 2013

Yea I use the useminPrepare task to create the concat config from my html
file. useminPrepare will overwrite any existing concat config for a file
so you can't just add the config in your grunt file manually (unless I
missed something).

On Fri, Mar 29, 2013 at 11:37 AM, Eric Clemmons notifications@github.comwrote:

Let me look into this real quick. I'm using usemin and concat now with no
problems.

Do you have a concat config setting at all, or is usemin creating that
behind the scenes for you?

Eric Clemmons
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Friday, March 29, 2013 at 9:34 AM, Chris Gross wrote:

If you use a task like grunt-usemin to pull your concat config from your
html file, then its not possible to just add a new line to your concat
config. Instead you want this to happen automatically.
So this adds a new option for ngtemplates to automatically have the dest
file added to an existing concat config.
You can merge this Pull Request by running
git pull https://github.com/cgross/grunt-angular-templates master
Or view, comment on, or merge it at:
#17
Commit Summary
New concat option to support automatic concat config addition

File Changes
M README.md (
https://github.com/ericclemmons/grunt-angular-templates/pull/17/files#diff-0)
(11)
M tasks/angular-templates.js (
https://github.com/ericclemmons/grunt-angular-templates/pull/17/files#diff-1)
(21)

Patch Links:
https://github.com/ericclemmons/grunt-angular-templates/pull/17.patch
https://github.com/ericclemmons/grunt-angular-templates/pull/17.diff


Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-15646265
.

@ericclemmons
Copy link
Owner

This is how I'm using it:

HTML: https://github.com/ericclemmons/genesis-skeleton/blob/master/src/public/index.html#L45
concat: https://github.com/ericclemmons/genesis-skeleton/blob/master/Gruntfile.coffee#L132
ngtemplates: https://github.com/ericclemmons/genesis-skeleton/blob/master/Gruntfile.coffee#L124

In short, I'm creating a single app.js file that contains all of my angular app (otherwise, my HTML would be littered with dozens of <script /> tags!). Then, usemin combines all of the script tags, the last of which is my previously-concated app.js.

If your case, the PR-less fix would be to use a simple concat task that combines your app with ngtemplates.app.dest (as seen above). The output would be all you'd need in your HTML.

Will this PR work with the gruntfile I have linked above? Where I have concat: { app: { src: [...] } }?

@cgross
Copy link
Author

cgross commented Mar 29, 2013

Yea I think we our project structures are a bit different. I do put lots
of tags in my index.html for all of my different controllers/services/etc.
I think it makes it easier when debugging during development. And I like
that the index.html is essentially a listing of my code being pulled in.
Its the 'one true source' for my code and dependencies.

I see that your doing two concats. The first one where you append the
template.js to your app.js and then another after useminPrepare runs. I
could do something similar and then that would alleviate the need for the
PR but it does feel less cluttered to me to allow ngtemplates to do it for
me. I don't even need concat config in my Gruntfile since I let usemin and
the PR to do the config.

How do you do daily development? Looking at your Gruntfile, I don't see
where you're concat-ing your stuff to app.js during the regarde/live-reload
stuff you have. Are you manually doing 'grunt prepare' after every change?

To use this new PR, you could just remove the concat config you have,
change your ngtemplates config to include

options: 'js/all.min.js'

Then put your ngtemplates after useminPrepare in your optimize alias. So
then the full list for the concat is the final list of useminPrepare +
ngtemplates. But maybe you need to leave the concat in place because you
do 'prepare' regularly...? and run your development stuff against your
concat-ed stuff.

So yea, we have different structures and I see how it might not be
something you would even use. Definitely makes my configuration simpler.
You running at development time with concat-ed and ngtemplate-d stuff
where I run with that all separate (and let angular load my templates via
xhr). Only when I run a dist build do I then want to use concat and
ngtemplate.

regards,
-Chris

On Fri, Mar 29, 2013 at 12:31 PM, Eric Clemmons notifications@github.comwrote:

This is how I'm using it:

HTML:
https://github.com/ericclemmons/genesis-skeleton/blob/master/src/public/index.html#L45
concat:
https://github.com/ericclemmons/genesis-skeleton/blob/master/Gruntfile.coffee#L132
ngtemplates:
https://github.com/ericclemmons/genesis-skeleton/blob/master/Gruntfile.coffee#L124

In short, I'm creating a single app.js file that contains all of my
angular app (otherwise, my HTML would be littered with dozens of
tags!). Then, usemin combines all of the script tags, the last of which is
my previously-concated app.js.

If your case, the PR-less fix would be to use a simple concat task that
combines your app with ngtemplates.app.dest (as seen above). The output
would be all you'd need in your HTML.

Will this PR work with the gruntfile I have linked above? Where I have concat:
{ app: { src: [...] } }?


Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-15648721
.

@ericclemmons
Copy link
Owner

Ah, I see now.

This project actually came about as a result of my work on http://genesis-skeleton.com/.

Because the concat task automatically combines everything under my /src/client/js folder, my index.html doesn't have to have a reference to every template & every JS file for my app: only the 3rd party scripts + my combined app.js file (which has the compiled templates added to it each time).

All of this runs with each change to work with LiveReload.

Let me take some time tonight to review the PR again. I'd like it to work well with usemin and the way you're doing it, but I'd have to test it out a bit.

Thanks!

@cgross
Copy link
Author

cgross commented Mar 29, 2013

Cool. Thats very similar to my approach with
https://github.com/cgross/angular-project-template. I'm gonna take a
longer look at it next week.

Thanks for looking at the PR. I think your ngtemplates task is great and
is definitely filling a void. Hope you can add this PR or a similar
feature.

On Fri, Mar 29, 2013 at 5:06 PM, Eric Clemmons notifications@github.comwrote:

Ah, I see now.

This project actually came about as a result of my work on
http://genesis-skeleton.com/.

Because the concat task automatically combines everything under my
/src/client/js folder, my index.html doesn't have to have a reference to
every template & every JS file for my app: only the 3rd party scripts + my
combined app.js file (which has the compiled templates added to it each
time).

All of this runs with each change to work with LiveReload.

Let me take some time tonight to review the PR again. I'd like it to work
well with usemin and the way you're doing it, but I'd have to test it out
a bit.

Thanks!


Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-15660727
.

@ericclemmons
Copy link
Owner

@cgross Just letting you know I didn't forget about this :) I'm finishing my vacation today and will be back home this weekend to address this PR.

@falsetto
Copy link

falsetto commented May 4, 2013

Thanks @cgross! This is exactly what I needed! I've pinned my package.json to your fork for now. Hoping this gets merged.

@ericclemmons
Copy link
Owner

It will! We've just been refactoring an internal project that could use this feature as well :)

Eric Clemmons
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Saturday, May 4, 2013 at 6:39 PM, Jedidiah Hurt wrote:

Thanks @cgross (https://github.com/cgross)! This is exactly what I needed! I've pinned my package.json to your fork for now. Hoping this gets merged.


Reply to this email directly or view it on GitHub (#17 (comment)).

@falsetto
Copy link

falsetto commented May 5, 2013

Great. Thanks for sharing this awesome project!

Sent from my iPhone

On May 4, 2013, at 7:46 PM, Eric Clemmons notifications@github.com wrote:

It will! We've just been refactoring an internal project that could use this feature as well :)

Eric Clemmons
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Saturday, May 4, 2013 at 6:39 PM, Jedidiah Hurt wrote:

Thanks @cgross (https://github.com/cgross)! This is exactly what I needed! I've pinned my package.json to your fork for now. Hoping this gets merged.


Reply to this email directly or view it on GitHub (#17 (comment)).


Reply to this email directly or view it on GitHub.

@ericclemmons
Copy link
Owner

I was just working on the PR for this (cleaning up the README examples to use the same paths & what not) and was wondering, is there any reason for this to be generic to modify any task?

For example, I cannot use this with my watch or regarde tasks because I'm using the expanded options (e.g. cwd, expand, src, dest, etc.) for locating files vs. useminPrepare's format, which is a simple dest: [ files ] format.

With that in mind, I was going to just make the option obvious & simple:

options: {
  base: ...
  prepend: ...
  usemin: 'path/to/build.js`
}

Sound good guys?

@ericclemmons
Copy link
Owner

It would also support usemin: [ 'first/build.js', 'second/build.js' ].

I don't want to change things up on you @cgross if you're modifying tasks besides usemin.

@cgross
Copy link
Author

cgross commented May 5, 2013

Hey guys - yea thats ok with me. I've actually moved away from usemin so I'm not even using this feature anymore. Still I do believe anyone continuing to use usemin would benefit by it.

I created grunt-dom-munger to do all the stuff usemin was doing while also being much more flexible (for ex, I needed to be able to exclude certain scripts from minification). https://github.com/cgross/grunt-dom-munger

@ericclemmons ericclemmons merged commit 8f84769 into ericclemmons:master May 6, 2013
@ericclemmons
Copy link
Owner

Just published v0.3.3! @cgross I was sure to use your commits so that you get credit. Would you like me to add you to the README as a contributor, or the package.json? (I haven't had very many contributors on projects, so not sure the best practice here :D)

I decided to make it simple: just specify the new option concat: 'target' and it adds it. Seems to work for both 'dest': ['files'] and src: ['files'] formats!

Thanks again fro the input fellas!

@cgross
Copy link
Author

cgross commented May 6, 2013

Props in the README would be awesome :) Sometimes you'll have a
contributors.md but maybe for smaller projects thats overkill. Either way
works for me!

On Mon, May 6, 2013 at 12:41 AM, Eric Clemmons notifications@github.comwrote:

Just published v0.3.3! @cgross https://github.com/cgross I was sure to
use your commits so that you get credit. Would you like me to add you to
the README as a contributor, or the package.json? (I haven't had very many
contributors on projects, so not sure the best practice here :D)

I decided to make it simple: just specify the new option concat: 'target'and it adds it. Seems to work for both 'dest':
['files'] and src: ['files'] formats!

Thanks again fro the input fellas!


Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-17466135
.

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.

3 participants