btford / ngmin

**deprecated** AngularJS Pre-minifier –> use ng-annotate –>
https://github.com/olov/ng-annotate
860 stars 42 forks source link

The future of ngmin and ng-annotate #93

Open olov opened 10 years ago

olov commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

eddiemonge commented 10 years ago

+1

btford commented 10 years ago

+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 commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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.

tlvince commented 10 years ago

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 commented 10 years ago

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 10 years ago

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 10 years ago

@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.

WhatFreshHellIsThis commented 10 years ago

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.

IgorMinar commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

mgol commented 10 years ago

See https://github.com/angular/angular.js/pull/8117

olov commented 10 years ago

ng-strict-di section added to ng-annotate README: https://github.com/olov/ng-annotate#highly-recommended-enable-ng-strict-di-in-your-minified-builds

IgorMinar commented 10 years ago

awesome!

On Thu, Jul 10, 2014 at 3:59 PM, Olov Lassus notifications@github.com wrote:

ng-strict-di section added to ng-annotate README: 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 https://github.com/btford/ngmin/issues/93#issuecomment-48675576.

WhatFreshHellIsThis commented 10 years ago

Typo? "Do that in your ng-annotate processed builds and AngularJS will let you in case there are any missing ..."

Did you mean warn you when you wrote let you?

olov commented 10 years ago

oops, thanks!

mbcooper commented 9 years ago

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

See xxxxx

olov commented 9 years ago

@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 commented 9 years ago

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 https://github.com/btford/ngmin/issues/93#issuecomment-64158246.