The future of ngmin and ng-annotate #93

Open
olov opened this Issue May 14, 2014 · 23 comments

Comments

Projects
None yet
10 participants
@olov

olov commented May 14, 2014

So I created ng-annotate because I needed it. I had some experience with AST fiddling from the past (a JS liveprogramming system, JSShaper, restrict mode, defs.js) and I decided to just sit down one night and write it. Then I shared it with others.

This is an attempt for me to answer the question "isn't there ngmin for that?" and for me to take part in providing an answer for "can you join forces?"

First of all, let's compare the two tools. If you feel that the comparison is unfair, please say so. And before that even, let me point out that I'm a big fan of AngularJS and its team, Brian included. I do not wish to make an impression that ngmin is bad or that it's badly written (neither is true). So lots of <3 and all that!

Where ng-annotate is "better" than ngmin

It treats your source code gently by only modifying things it should.

It can remove (and rewrite) annotations (gently), great for de-cluttering an existing code base or for those who like their annotations checked in but are about to do a major refactoring.

It is very (very!) fast by design: single-pass analysis, single-pass textual rewrite, imperative matching, careful attention to big-O performance as well as the occasional micro-optimization. More than 33x speedups have been reported.

It is actively maintained.

It supports user plugins. More info

It supports explicit annotations via /* @ngInject */ comments. More info

It understands many more angular declaration forms, including ui-router. More info

It passes its test suite (duh). It also passes all ngmin tests.

It fixes all reported ngmin "bug" issues. To be precise, 24 open ngmin issues just work in ng-annotate (clickable links), 6 open ngmin issues have a decent workaround in ng-annotate using /* @ngInject */ (clickable links), the remaining 6 open ngmin issues are feature requests (none of which are implemented in ng-annotate). Full annotated list

It has 0 open issues (as of writing this) and I've promised to fix any raised issues promptly (at minimum with a decent workaround). You can judge my track record for yourself by looking through the closed issues.

Where ng-annotate is "worse" than ngmin

edit: updated list, ng-annotate and ngmin are now roughly on par in terms of tooling support so I don't consider this to be an issue any longer. It does not (yet) have plugins for as many tools. It does have Grunt, Browserify, Brunch, Gulp, Broccoli and Rails asset pipeline integrations and more will come.

It does not have ngmin-dynamic, but since this is a separate program it can be integrated with ng-annotate just like it is integrated with ngmin. I'm not sure how stable it is and to what extent it is used yet.

It has a slightly larger risk of detecting false positives. It's a trade-off with flexibility and it is intentional. Having said that, any reported real-world issue will get fixed. In particular, ng-annotate supports myMod.controller("foo", function() {}) no matter where (or how) myMod is declared. For the rare situation where this would cause an issue, the user can disable it altogether or limit it to certain names by passing an option to ng-annotate. It can default to off if this ever becomes an issue but right now it isn't.

ng-annotate's future

I will continue maintaining ng-annotate. I will do this because for the vast majority of AngularJS users it is the better choice (the notable exception being when a tool integration is lacking). I rely on it every day in production, and lots of ng-annotate users does too. Although ngmin is by far the most well known and common choice, the ng-annotate user base is increasingly growing and it seems most of that consist of users moving from ngmin to ng-annotate.

I will continue improving ng-annotate.

I have not made a huge amount of noise regarding ng-annotate so far other than through my own communication channels but now that it is about to reach v1.0 I plan to evangelize it a little bit more. Specifically, I will ask the different boilerplate/scaffolding-projects to consider supporting ng-annotate and I will ask for it to be included in documentation whenever applicable. I will also pay more attention to tooling integration, perhaps contributing a missing plugin or two where it makes the biggest impact.

Join forces?

Can we join forces? I've been hesitant to ask this question because in a weird way it kinda sorta implies asking Brian to "deprecate" his code. Brian, I love your recent stuff like zone.js by the way. Did I say that I'm a fan of Brian and the AngularJS team? :)

I can't provide ng-annotate as a bunch of pull requests to ngmin, unless the end result effectively is a code import. For ng-annotate to be ng-annotate (or for ngmin to become ng-annotate), its code will need to resemble ng-annotate to a very large extent. See "fast by design" above. ng-annotate is great for the reasons it's great (WAT?!) and I won't compromise with its performance because it is as important to me as it is to any other user. The ng-annotate code base is in good shape and very hackable. Having said that, I do appreciate the elegance of the ngmin code base, but as already stated - the ng-annotate code looks vastly different for a reason.

So what can I do? I can politely, with all humbleness and respect, ask Brian whether it would make any sense to appoint ng-annotate as the successor to ngmin. In the best of worlds the end result would yield more time for Brian to hack on other AngularJS awesomeness instead of rewriting ngmin, and happier end-users because they get a more capable tool and faster bug fixes. I do realize that the tooling integration situation may be a blocker for this and I am willing to work on that actively (perhaps with a little bit of help).

If this would happen Brian would obviously get a commit bit if he wanted. I would be willing to move the project into its own organization or whatever you would feel necessary as well. I've already promised to continue maintaining ng-annotate (see ng-annotate's future) so nothing would change from that perspective.

If a pre-requisite for this to happen would be renaming ng-annotate to ngmin, sure so be it even though it isn't very descriptive of what ng-annotate does. The ng-annotate command-line and library API can't become identical to the ngmin API so I'm not sure that a rename is the best idea, but compared to everything else the name is irrelevant so I'm sure willing to discuss that.

So…

I want to end by saying that even though the answer I hope for is "yes", I have full respect for a "no". In either case, I'll just continue hacking on and if nothing else we'll have answers to the two questions this all started with.

Brian, can ng-annotate become the successor to ngmin?

@gabrielmaldi

This comment has been minimized.

Show comment
Hide comment
@gabrielmaldi

gabrielmaldi May 14, 2014

I'd like to point out that a couple of days ago I decided to use Grunt for building my current project. After selecting many packages for other tasks, I had to eventually decide: "do I go with ng-annotate or ngMin?". I tried both and neither one fit exactly into my scenario out of the box, so I started experimenting with their options, messing with their output using text-replace, and filed a few issues. In the end I chose ng-annotate because I felt it was more easily customizable and also because @olov addressed all my 3 issues strikingly fast and positively (fair is to say, of course, that I didn't submit any issues to ngMin, so I can't speak for that). I am happy with my decision as I was able to make ng-annotate work exactly like I wanted it to for me.

I'd like to point out that a couple of days ago I decided to use Grunt for building my current project. After selecting many packages for other tasks, I had to eventually decide: "do I go with ng-annotate or ngMin?". I tried both and neither one fit exactly into my scenario out of the box, so I started experimenting with their options, messing with their output using text-replace, and filed a few issues. In the end I chose ng-annotate because I felt it was more easily customizable and also because @olov addressed all my 3 issues strikingly fast and positively (fair is to say, of course, that I didn't submit any issues to ngMin, so I can't speak for that). I am happy with my decision as I was able to make ng-annotate work exactly like I wanted it to for me.

@capaj

This comment has been minimized.

Show comment
Hide comment
@capaj

capaj May 14, 2014

+1 for ng-annotate, Brian focus on other more important stuff, leave ngmin behind

capaj commented May 14, 2014

+1 for ng-annotate, Brian focus on other more important stuff, leave ngmin behind

@eddiemonge

This comment has been minimized.

Show comment
Hide comment
@eddiemonge

eddiemonge May 16, 2014

Collaborator

+1

Collaborator

eddiemonge commented May 16, 2014

+1

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford May 21, 2014

Owner

+1000; @olov would you like to take over this repo? I'd gladly add you as a contributor on this GitHub repo and on NPM.

I think a reasonable path forward would be to have ngmin use ng-annotate internally for the static mode and ngmin-dynamic for the dynamic mode. Then anyone using this module (or a module wrapping it) can continue doing so. I'm fine with deprecating this code, but I'd like to do so in a way that's painless for those using it.

Owner

btford commented May 21, 2014

+1000; @olov would you like to take over this repo? I'd gladly add you as a contributor on this GitHub repo and on NPM.

I think a reasonable path forward would be to have ngmin use ng-annotate internally for the static mode and ngmin-dynamic for the dynamic mode. Then anyone using this module (or a module wrapping it) can continue doing so. I'm fine with deprecating this code, but I'd like to do so in a way that's painless for those using it.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 21, 2014

@btford What's the exact purpose of ngmin-dynamic? README doesn't say a lot.

Since ngmin's API is a subset of ngAnnotate's one, delegation should be easy. Then the grunt-ngmin plugin wouldn't even require changes. And if anyone wants to use more of the API, there's my grunt-ng-annotate plugin.

As for the migration itself, the regular grunt-ngmin config should just work with ngmin swapped to ngAnnotate, the default options are the same. Perhaps READMEs could have pointers to respective ngAnnotate projects for further options?

mgol commented May 21, 2014

@btford What's the exact purpose of ngmin-dynamic? README doesn't say a lot.

Since ngmin's API is a subset of ngAnnotate's one, delegation should be easy. Then the grunt-ngmin plugin wouldn't even require changes. And if anyone wants to use more of the API, there's my grunt-ng-annotate plugin.

As for the migration itself, the regular grunt-ngmin config should just work with ngmin swapped to ngAnnotate, the default options are the same. Perhaps READMEs could have pointers to respective ngAnnotate projects for further options?

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov May 21, 2014

Thanks Brian!

Can we start the initial step of the migration by adding a note in the ngmin README and issue tracker that points them to ng-annotate? That way we'll help the most active ngmin users moving to ng-annotate. Switching should be straightforward for most of them (build tool integration is soon on par) and I'll add a section to the ng-annotate README that addresses it specifically. I'll also add support to ng-annotate for reading from stdin and writing to a named file to further help switching.

As a step two, the boilerplate/generator projects (most notably generator-angular (yeoman)) and angular documentation should move to/mention ng-annotate, to get new users going in the right direction. I think it makes sense to wait a couple of weeks with step two, to give us time to address any new issues coming from new users as a consequence of step one. (early adopters and some tooling are already moving to ng-annotate, see upcoming ngbp v0.4 and possibly future assetgraph).

As a step three, say a couple of months later, let's see what we should do about the remaining ngmin users. If a significant amount of the users are staying with ngmin and we want to push them over, then let's pick up the possibility of doing a new ngmin release that wraps ng-annotate. We'd have to think about this carefully because we can't do it in a way that's fully immune against regressions and build failures. IMO it would be better if people gradually moved over and even if some people stay on for a little longer with ngmin - as long as they are happy with it then that's not a problem. When they aren't any longer, they'll hit the issue tracker and will learn about ng-annotate.

Is that a viable approach?

olov commented May 21, 2014

Thanks Brian!

Can we start the initial step of the migration by adding a note in the ngmin README and issue tracker that points them to ng-annotate? That way we'll help the most active ngmin users moving to ng-annotate. Switching should be straightforward for most of them (build tool integration is soon on par) and I'll add a section to the ng-annotate README that addresses it specifically. I'll also add support to ng-annotate for reading from stdin and writing to a named file to further help switching.

As a step two, the boilerplate/generator projects (most notably generator-angular (yeoman)) and angular documentation should move to/mention ng-annotate, to get new users going in the right direction. I think it makes sense to wait a couple of weeks with step two, to give us time to address any new issues coming from new users as a consequence of step one. (early adopters and some tooling are already moving to ng-annotate, see upcoming ngbp v0.4 and possibly future assetgraph).

As a step three, say a couple of months later, let's see what we should do about the remaining ngmin users. If a significant amount of the users are staying with ngmin and we want to push them over, then let's pick up the possibility of doing a new ngmin release that wraps ng-annotate. We'd have to think about this carefully because we can't do it in a way that's fully immune against regressions and build failures. IMO it would be better if people gradually moved over and even if some people stay on for a little longer with ngmin - as long as they are happy with it then that's not a problem. When they aren't any longer, they'll hit the issue tracker and will learn about ng-annotate.

Is that a viable approach?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 21, 2014

Also, if there are cases in the Grunt task that work fine in grunt-ngmin but fail in grunt-ng-annotate, please report it to me! If that's not too breaking, I'll try to remaing as compatible as possible.

mgol commented May 21, 2014

Also, if there are cases in the Grunt task that work fine in grunt-ngmin but fail in grunt-ng-annotate, please report it to me! If that's not too breaking, I'll try to remaing as compatible as possible.

olov added a commit to olov/ng-annotate that referenced this issue May 21, 2014

@tlvince

This comment has been minimized.

Show comment
Hide comment
@tlvince

tlvince May 21, 2014

As for the migration itself, the regular grunt-ngmin config should just work with ngmin swapped to ngAnnotate, the default options are the same

Just to chime in and say we migrated from (grunt-)ngmin to (grunt-)ng-annotate painlessly in a yeoman/generator-angular scaffolded project; it was a drop-in replacement, with a simple s/ngmin/ngAnnotate/ in Gruntfile.

tlvince commented May 21, 2014

As for the migration itself, the regular grunt-ngmin config should just work with ngmin swapped to ngAnnotate, the default options are the same

Just to chime in and say we migrated from (grunt-)ngmin to (grunt-)ng-annotate painlessly in a yeoman/generator-angular scaffolded project; it was a drop-in replacement, with a simple s/ngmin/ngAnnotate/ in Gruntfile.

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov May 21, 2014

Thanks for sharing that. Updating an existing (scaffolded or not) Gruntfile from ngmin to ng-annotate seems simple enough for users to do themselves.

An update on my previous comment: There's now code in master (will be part of ng-annotate 0.9.5) for reading from stdin and writing to a file.

olov commented May 21, 2014

Thanks for sharing that. Updating an existing (scaffolded or not) Gruntfile from ngmin to ng-annotate seems simple enough for users to do themselves.

An update on my previous comment: There's now code in master (will be part of ng-annotate 0.9.5) for reading from stdin and writing to a file.

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov May 23, 2014

ng-annotate 0.9.5 is now released with a "Migrating from ngmin" README section. I've also updated the initial issue description to reflect that ng-annotate tooling integration is now roughly on par with ngmin.

olov commented May 23, 2014

ng-annotate 0.9.5 is now released with a "Migrating from ngmin" README section. I've also updated the initial issue description to reflect that ng-annotate tooling integration is now roughly on par with ngmin.

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov May 23, 2014

@btford if you think my proposed three-step migration plan makes sense then now would be a good time to push an updated ngmin README to github (and npm?) and to close the open ngmin issues with a comment about ng-annotate.

olov commented May 23, 2014

@btford if you think my proposed three-step migration plan makes sense then now would be a good time to push an updated ngmin README to github (and npm?) and to close the open ngmin issues with a comment about ng-annotate.

@eddiemonge eddiemonge self-assigned this May 26, 2014

tlvince added a commit to tlvince/LMIS-Chrome that referenced this issue May 27, 2014

@tlvince tlvince referenced this issue in eHealthAfrica/LMIS-Chrome May 27, 2014

Merged

Replace ngmin with ng-annotate, closes item:456 #331

@WhatFreshHellIsThis

This comment has been minimized.

Show comment
Hide comment
@WhatFreshHellIsThis

WhatFreshHellIsThis Jun 9, 2014

Just wanted to report that I have a fairly large Angular app and ngmin aside from being slow failed to prepare it for minification properly and all I got was a blank page when I launched my app, couldn't determine where in the time I had (mostly it must be said due to Angular's lack of error reporting in these situations, not ngMin's fault directly).

Just did a drop in replacement with ngAnnotate and aside from being incredibly faster it seems to have worked perfectly to prepare my app for minification where ngmin did not. Instead of a blank page my app loads and runs properly.

Just wanted to report that I have a fairly large Angular app and ngmin aside from being slow failed to prepare it for minification properly and all I got was a blank page when I launched my app, couldn't determine where in the time I had (mostly it must be said due to Angular's lack of error reporting in these situations, not ngMin's fault directly).

Just did a drop in replacement with ngAnnotate and aside from being incredibly faster it seems to have worked perfectly to prepare my app for minification where ngmin did not. Instead of a blank page my app loads and runs properly.

olov added a commit to olov/ng-annotate that referenced this issue Jun 12, 2014

@mrzmyr mrzmyr referenced this issue in diegonetto/generator-ionic Jun 16, 2014

Closed

Replace ngmin with ngAnnotate #59

@JaKXz JaKXz referenced this issue in angular-fullstack/generator-angular-fullstack Jun 16, 2014

Closed

ngmin vs ngAnnotate #266

@doubleo2 doubleo2 referenced this issue in ngbp/ngbp Jun 17, 2014

Closed

Replace ngmin with ng-annotate #276

@awashbrook awashbrook referenced this issue in dtpublic/malhar-angular-dashboard Jun 30, 2014

Merged

Fixed issues with minification #37

@pdudkiewicz pdudkiewicz referenced this issue in linemanjs/lineman-angular Jul 8, 2014

Closed

Migrate to ng-annotate #9

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Jul 9, 2014

Contributor

I've never been a fan of ngmin because it wasn't 100% reliable due to the lack of context available to the tool. But looking at ng-annotate it does look like a more robust solution especially when combine with the strict-di mode (see ngApp docs).

+1 for moving forward with the deprecation of ngmin in favor of ng-annotate, but I'd also like a strong recommendation that people use it in combination with the strict-di mode, only then can we be sure that all the code is minification-safe.

Contributor

IgorMinar commented Jul 9, 2014

I've never been a fan of ngmin because it wasn't 100% reliable due to the lack of context available to the tool. But looking at ng-annotate it does look like a more robust solution especially when combine with the strict-di mode (see ngApp docs).

+1 for moving forward with the deprecation of ngmin in favor of ng-annotate, but I'd also like a strong recommendation that people use it in combination with the strict-di mode, only then can we be sure that all the code is minification-safe.

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov Jul 9, 2014

I'm looking forward to strict-di. I'll add info about it to the ng-annotate README. Due for 1.3, right? Thanks for your support!

olov commented Jul 9, 2014

I'm looking forward to strict-di. I'll add info about it to the ng-annotate README. Due for 1.3, right? Thanks for your support!

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jul 9, 2014

@olov strict-di is already implemented in latest 1.3.0 betas and will be shipped with 1.3.0 stable.

mgol commented Jul 9, 2014

@olov strict-di is already implemented in latest 1.3.0 betas and will be shipped with 1.3.0 stable.

mgol added a commit to mgol/angular.js that referenced this issue Jul 9, 2014

docs(guide): switch from ngmin to ng-annotate
ng-annotate is an independent alternative to ngmin that is non-invasive
and more performant. For the background around the switch, see the discussion
at:
btford/ngmin#93

@mgol mgol referenced this issue in angular/angular.js Jul 9, 2014

Closed

docs(guide): switch from ngmin to ng-annotate #8117

@mgol

This comment has been minimized.

Show comment
Hide comment

mgol added a commit to angular/angular.js that referenced this issue Jul 10, 2014

docs(guide): switch from ngmin to ng-annotate
ng-annotate is an independent alternative to ngmin that is non-invasive
and more performant. For the background around the switch, see the discussion
at:
btford/ngmin#93

Closes #8117
@olov

This comment has been minimized.

Show comment
Hide comment

This was referenced Aug 7, 2014

@brunocoelho brunocoelho referenced this issue in cironunes/angular-off-canvas Aug 28, 2014

Merged

chore(gulp): change deprecated gulp task #25

@meriadec meriadec referenced this issue in balthazar/ng-markdown Sep 1, 2014

Closed

use gulp-ng-annotate instead of gulp-ng-min #17

@davisford davisford referenced this issue in jvandemo/angular-growl-notifications Sep 15, 2014

Closed

undefined is not a function when minified #3

@mbcooper

This comment has been minimized.

Show comment
Hide comment
@mbcooper

mbcooper Nov 24, 2014

@olov. I have an example of an ng-min to ng-annotate switch that doesn't work.

See xxxxx

@olov. I have an example of an ng-min to ng-annotate switch that doesn't work.

See xxxxx

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov Nov 24, 2014

@mbcooper you can file issues at https://github.com/olov/ng-annotate/issues but please provide a minimal example of input, actual output and expected output (both non-minified), together with the version of ng-annotate you are using. The simplest way to debug minification issues with Angular 1.3+ is using ng-strict-di, see https://github.com/olov/ng-annotate#highly-recommended-enable-ng-strict-di-in-your-minified-builds.

olov commented Nov 24, 2014

@mbcooper you can file issues at https://github.com/olov/ng-annotate/issues but please provide a minimal example of input, actual output and expected output (both non-minified), together with the version of ng-annotate you are using. The simplest way to debug minification issues with Angular 1.3+ is using ng-strict-di, see https://github.com/olov/ng-annotate#highly-recommended-enable-ng-strict-di-in-your-minified-builds.

@mbcooper

This comment has been minimized.

Show comment
Hide comment
@mbcooper

mbcooper Nov 24, 2014

My bad. Was using 0.4.0

0.7.0 worked very nicely!

Thanks!

--mike

On Mon, Nov 24, 2014 at 2:22 AM, Olov Lassus notifications@github.com
wrote:

@mbcooper https://github.com/mbcooper you can file issues at
https://github.com/olov/ng-annotate/issues but please provide a minimal
example of input, actual output and expected output (both non-minified),
together with the version of ng-annotate you are using. The simplest way to
debug minification issues with Angular 1.3+ is using ng-strict-di, see
https://github.com/olov/ng-annotate#highly-recommended-enable-ng-strict-di-in-your-minified-builds
.


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

My bad. Was using 0.4.0

0.7.0 worked very nicely!

Thanks!

--mike

On Mon, Nov 24, 2014 at 2:22 AM, Olov Lassus notifications@github.com
wrote:

@mbcooper https://github.com/mbcooper you can file issues at
https://github.com/olov/ng-annotate/issues but please provide a minimal
example of input, actual output and expected output (both non-minified),
together with the version of ng-annotate you are using. The simplest way to
debug minification issues with Angular 1.3+ is using ng-strict-di, see
https://github.com/olov/ng-annotate#highly-recommended-enable-ng-strict-di-in-your-minified-builds
.


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

timothykang added a commit to VOLTTRON/openeis-ui that referenced this issue Feb 13, 2015

@eddiemonge eddiemonge removed their assignment May 14, 2015

Repository owner locked and limited conversation to collaborators May 14, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.