ericclemmons / grunt-angular-templates

Grunt build task to concatenate & pre-load your AngularJS templates
MIT License
710 stars 107 forks source link

Make usemin option work without uglify. #69

Closed bgoldsworthy closed 8 years ago

bgoldsworthy commented 10 years ago

I'm embedding my angular app inside a .NET MVC app, and want to let MVC's bundling handle the minification, so I took uglify out of the usemin flow and am just using it for concatenation.

This change to the ngtemplates appender falls back on looking for the concat.generated task and appending its template concatenation to there instead.

I tried to write tests, but couldn't convince the usemin task to build both uglified and non-uglified files.

ericclemmons commented 10 years ago

Howdy @bgoldsworthy! It took a couple of reads, but what you're saying is that when you don't use uglify, this project breaks?

It's probably a stupid question, but if you're just using the usemin for concatenation, why not just have ngtemplates use the concat option instead of the usemin option?

(Trying to make sure I understand the difference here)

Thanks!

bgoldsworthy commented 10 years ago

I still have usemin generate the concat task since it will do the script tag replacement for me and its syntax is easier to manage grouping files into several concatenated files.

As a result, I don't have an explicit concat task in grunt to hook into, only usemin's concat:generated. I see now that I could create a concat:templates task that runs last and appends the ngtemplate file to the final usemin output and then specify that only concat:generated gets run in the usemin subtask, which should work fine.

Or maybe I'm missing something and there's an easier way to have the concat option hook directly into the concat.generated task?

ericclemmons commented 10 years ago

Ahh I see. Yea, I'm personally not a fan of usemin, and prefer concat'ing "app/*/.js" into a single file.

Either way, I think I understand your use case now. So I can work on the test, can you paste the task( s) that are ran during your build? It's odd that yours doesn't create an uglify target, but that may because you don't have the task defined or module loaded.

bgoldsworthy commented 10 years ago

It doesn't create an uglify target because I've taken uglify out of usemin's flow. Usemin uses the flow to decide when to move the files out of the .tmp directory and into their final destination. Here's how, if you're interested.. So if the flow isn't changed, concat:generated moves the files to .tmp/uglify and they just sit there, because uglify:generated is responsible for moving them into the dest directory.

Here's the relevant sections of my Gruntfile

grunt.initConfig({
   useminPrepare: {
      html: ['app/*.html'],
      options: {
        dest: 'dist',
        flow: { steps: { 'js': ['concat'], 'css': ['concat']}, post: {}}
      }
    },
    usemin: {
      html: ['dist/*.html']
    }
 });

 grunt.registerTask('build', ['useminPrepare', 'concat', 'usemin']);  
ericclemmons commented 10 years ago

Ah, thanks for this! I've never used the flow option. Now it makes sense :)

I should be able to create a test-case for this, at least to ensure it doesn't break with future changes ;)

SimplGy commented 10 years ago

I'm in the same boat, that's how I found this issue. It's nice to be able to let usemin concat only for dev and dev integration servers, and do the full concat+uglifyjs for dist and qa servers.

Because ngtemplates looks for the configuration in the uglifyjs configuration object, it doesn't play well with usemin in a concat-only mode. Looks like you've figured that out :)

+1

SimplGy commented 10 years ago

I'm surprised anyone can use a **/*.js method reliably--doesn't that mean you have no control over source order? And don't you need control over source order in Angular if you don't use RequireJS? (for example, to declare the module before you add things to it)

ericclemmons commented 10 years ago

@SimpleAsCouldBe Really? Well, that's not surprising. Most of the Angular apps I've seen encouraged & developed (especially the Yeoman generators & seed projects) build on a single app, which is just wrong.

Check out the demo (you may need to refresh for it to work right) of another Angular project I'm working: grunt-angular-modularize simple example.

There should be 1 module per file, so that concat, requirejs, and browserify can all build reliably, where each module exists independent of the other.

The problem is, most people who are building SPAs (using Angular especially) haven't build large, complex applications with route-specific builds.

SimplGy commented 10 years ago

Ya, separate files rock. I'd heard that the angular team was going to do optimized (non-single file) builds based on views instead of modules and recommend single module apps, but that's tangental to this issue.

Looking at your example, doesn't this line:

angular.module('app', [ 'chieffancypants.loadingBar', 'app.routes.home' ])

...Imply a giant explosion if you don't load the files that define app.routes.home module and chieffancypants.loadingBar module first?

ericclemmons commented 10 years ago

@SimpleAsCouldBe Super old message, I know, but dependencies are evaluated at application instantiation, not as evaluated.

This is why I've long been a proponent of each-file-being-a-module so that order of concatenation doesn't matter.

https://github.com/ericclemmons/grunt-angular-modularize

SimplGy commented 10 years ago

Thanks for the clarification, @ericclemmons. I didn't know that at the time. If JS load order is unimportant for things defined as angular modules and all that matters is the dependency tree, it seems like all is right with the world.

If angular module dependencies are evaluated at instantiation time, why aren't directive/service/factory dependencies evaluated at instantiation time, too? Or are they?

If everything was evaluated at instantiation time, there'd be no load order issue at all, unless I'm missing something.

ericclemmons commented 10 years ago

This is getting a bit too abstract, so it's easier to just say it with code. Dependency order only matters if you rely on a module's prior declaration to work:

// app.concat.js

// via: app.directive.foo.js
angular.module('app').directive('foo', ...);

// via: app.js
angular.module('app', ['ui.router']).config(...);

Despite being in separate files, this creates an implicit dependency on the order. Implicit dependencies should be in the same file to avoid issues with order.

Alternatively, I recommend making each file it's own module:

// app.concat.js

// via: app.directive.foo.js
angular.module('app.directive.foo', []).directive('foo', ...);

// via: app.js
angular.module('app', ['app.directive.foo', ''ui.router']).config(...);

Each file is now it's own module, and dependencies are explicit now.

SimplGy commented 10 years ago

This is tangental to the github issue, but I wanted to make sure of things so I did some experiments on order of operations in Angular. My findings are simple and welcome:

In Angular, source order does not matter.

As long as we declare our directives, factories, controllers, and so on to Angular's DI system, it will load them in the correct order. It has no problem if the source order doesn't match the load order. This applies if we're talking about module dependencies or services. 3rd party libraries, of course, are a separate beast.

Here is my test case:

http://plnkr.co/edit/oDtbNn?p=preview

// Works, even though app2 is declared later.
var app1 = angular.module( 'app1', ['app2'] );
var app2 = angular.module( 'app2', [] );

app1.run(function(){ console.log('app1') }); // runs second
app2.run(function(){ console.log('app2') }); // runs first
console.log('');

// Works, even though service2 is declared later.
app1.factory("service1", function(service2){
  console.log("service1");
});
app1.factory('service2', function(){
  console.log('service2');
});

app1.run(function(service1){});
ericclemmons commented 10 years ago

@SimpleAsCouldBe That's correct, module order doesn't matter, because the DI handles the resolution. But, file order for a single module does, which is what trips most people up when they concat files that are all trying to modify a module that doesn't exist yet :) Why can't Angular just lazily handle module dependency declarations!?

ericclemmons commented 10 years ago

Wait wait wait, @bgoldsworthy. We got off on a crazy foot here. Shouldn't I just merge this down & be done with it!?

bgoldsworthy commented 10 years ago

It looks like you were going to add tests to my shot at managing the flow targets, rather than just merging it in, since I couldn't get multiple versions of the same tasks running for some reason. All the Travis checkmark says is that I didn't break any of yours, right?

@SimpleAsCouldBe You can enforce source order using concat in the handful of files where it's important, by just listing your js files in concat's src list before **/*.js. Which makes more sense than usemin almost all of the time.

e.g.:

concat:{
src= [ 
  'src/templates.js', 
  'src/app.js', 
  'src/modules/mod1.js',  
  'src/**/*.js', 
  '!src/**/*.spec.js'
]}

So if app and mod1 are split across several files I make sure the ones that contain the module definition (angular.module('app', [])) get loaded before anything that tries to add controllers or directives to the module, but modules that are contained in a single file, I don't have to worry about, and the **/*.js grabs them.

I do still find usemin clearer when it's necessary to create multiple js files, either for reuse or late-loading, as I did when I opened this issue. But for just enforcing source order in a single file, concat is all you need, and much easier.

SimplGy commented 10 years ago

@bgoldsworthy That's a fantastic way to do it for concat. It makes sense because this kind of globbing only matches each file once, right, so there's no double inclusion.

SimplGy commented 10 years ago

@ericclemmons I'm pointing out that file/declaration order doesn't seem to matter for either modules OR services/factories/directives/controllers. I can write these in whatever source order I want and the DI system has no problem with it.

ericclemmons commented 10 years ago

@SimpleAsCouldBe There's a misunderstanding. :) Of course order doesn't matter for modules or features on that module. Order does matter for module instantiation. As in, the angular.module('app', []) file must come first, before any other file references angular.module('app') for modification, as the module has yet to be defined with dependencies.

That's why I always recommended 1-file-1-module so order truly doesn't matter, otherwise you'd have to do glob-foo to include app.js first and then exclude !app.js from all other targets like @bgoldsworthy showed above.

angular.module('foo').service('foo', function() {});
angular.module('foo', []);
// Uncaught Error: [$injector:nomod] http://errors.angularjs.org/1.3.0-beta.17/$injector/nomod?p0=foo 
bgoldsworthy commented 10 years ago

@SimpleAsCouldBe What @ericclemmons is saying is that when you split your module across several files, your module DECLARATION needs to be included before any additional files that add controllers/services/whatever to it.

mod1.js
angular.module('mod1',[]) // THIS is the module declaration and needs to happen first
.directive('mod1dir', function(mod1svc){
...
});

mod1-svc.js
angular.module('mod1') // THIS is the module getter and needs to occur AFTER the declaration
.service('mod1svc', function(){
...
});

In this case mod1.js needs to be loaded first, but as you're correctly saying, the other two files can be loaded in any order even though the directive depends on the service, because Angular's DI system can figure it out.

mod1.js
angular.module('mod1',[]);

mod1-dir.js
angular.module('mod1')
.directive('mod1dir', function(mod1svc){
...
});

mod1-svc.js
angular.module('mod1') // THIS is the module getter and needs to occur AFTER the definition
.service('mod1svc', function(){
...
});

Note that this is using angular.module as a getter, and avoiding using global vars for the modules as your example does, but it's similarly true if you're attaching a service to your app1 global that var app1 = angular.module('app1',[]); needs to happen before app1.controller(...)

SimplGy commented 10 years ago

I understand. I didn't test for order between module declaration and attaching behaviors. of course you're right, that is order dependent. (I wonder if it actually has to be though... Hmmm... That might be a pretty minor PR)

@ericclemmons: I see from your repo and comments that you put a lot of thought into the many-module perspective. Help me understand it?

In a single file stitch-min build (or a 2 file, where 1 file is your app and 1 file is vendor code), the only angular code that is order dependent is your module declaration. This can go in index.html, or with your vendor stuff (which is all or mostly order specific). Because of this, I don't see module splitting solving any significant order problem. You recommend it even in a single file build case?

In a multi-file build which lazy-loads content based on functionality there might be more benefit? Say, an admin section, a gallery section, and a contributor section, files which could be stitch-min'd separately (admin.min.js, gallery.min.js, contributor.min.js) without having to worry about load order would be nice. The gap between having a module for every file when what I probably want is a module+file per functional section is hard for me to bridge though.

JobaDiniz commented 9 years ago

I using only concat as well and ngtemplate does not work. Have workaround this?

var useminPrepare = {
        options: {
            flow: {
                telenor: {
                    steps: {
                        js: ['concat'],
                        css: ['concat', 'cssmin']
                    },
                    post: {}
                },
                technetennis: {
                    steps: {
                        js: ['concat'],
                        css: ['concat', 'cssmin']
                    },
                    post: {}
                }
            }
        },
        telenor: {
            src: "<%= config.copy %>/index-telenor.html",
            options: {
                dest: "<%= config.temp %>"
            }
        },
        technetennis: {
            src: "<%= config.copy %>/index-technetennis.html",
            options: {
                dest: "<%= config.temp %>"
            }
        }
    };
ericclemmons commented 9 years ago

TBH, I'm confused as to the current state of things here :)

If I had to guess, it's an issue unknown to me since each file is it's own module, so the order of concatenation does not matter.

Anybody else know what to do next here?

pgp44 commented 8 years ago

Hello, is this going somewhere? Still running into this issue and while the patch as such fixes the problem, it would be nice to be able to pull this from the official repository.

Tx!

ericclemmons commented 8 years ago

CC'ing the new owners... @underscorebrody @hvdb @aaani

underscorebrody commented 8 years ago

I opened a new PR with this change cherry-picked onto it plus some modifications to our tests that will allow us to run with or without uglification. Closing this PR in favor of https://github.com/ericclemmons/grunt-angular-templates/pull/153