Can we improve ng-annotate?

[olov]

Hi! The README says "Handles explicit annotation more gracefully than ng-annotate" and I'm interested in knowing more about it -- perhaps ng-annotate can also be improved? If you would be able to provide an example here or open up an issue on ng-annotate that would be awesome. Thanks!

[bholloway]

Hi @olov. First of all much respect for ng-annotate.

The history of nginject-loader begins with browserify-inject and its use in the Angularity tool.

Essentially angularity is global install to give our developers get a turn-key Browserify/BabelJS/SASS solution for all their AngularJS projects. It is getting a bit old now and is locked in to some old versions of these packages.

With Angularity we try to minify all the time. This is to eliminate any surprises going to production but places a heavily reliance on source-maps. Back when we were first developing there were some issues with source-maps coming out of ng-annotate. I did lots of testing with sokra's analysis tool but for whatever reason, I could not get clean source maps through annotation.

Certainly this not currently the case. I am using ng-annotate with Webpack and there are no issues. But this is with much more recent versions of babel, sass, and ng-annotate all.

The other side of the issue is the part you will be more interested in. We found we were heavily leveraging require() to separate our implementations from a single composition root that contains Angular. Take a look at this small example. And the resulting @ngInject can be seen in this example controller.

As far as I know it is still the case with ng-annotate that one would need to write the following, somewhat obtuse code.

/**
 * <p>Routing for the to-do app.</p>
 * @param {object} $StateProvider
 * @param {object} $urlRouterProvider
 */
function /**@ngInject**/ todoRoutes($stateProvider, $urlRouterProvider) {

There may have also been an issue at the time with support for ES6 syntax.

/**
 * ... 
 *@ngInject
 */
export default function ...

And

/**
 * ... 
 *@ngInject
 */
export default class ...

I did not attempt a PR at the time because the code was pretty impenetrable, I'm not sure if it is more modular now. Pre-minification is generally not a problem that needs extensibility so please don't take that as a criticism. My Esprima implementation is much slower so whatever your implementation it is certainly more effective.

If any of these issues still exist I would certainly like to do a PR for the sake of having contributed to such a ubiquitous tool. Just point me in the right direction.

[olov]

Thanks for that!

We used to have problems with the sourcemaps support. The reason for that was partly that we weren't willing to accept a huge slow down due to sourcemaps-processing so we needed a bit of creativity and time to smoke out the bugs. Now it works pretty well (and is fast!).

You do not have to place /*@ngInject*/ just before the function name when using ng-annotate, so your example works unchanged in ng-annotate (no need for the obtuse version). This has worked all the way back to ng-annotate 0.9.0 when I first implemented support for /*@ngInject*/.

This input:

/**
 * <p>Routing for the to-do app.</p>
 * @ngInject
 * @param {object} $StateProvider
 * @param {object} $urlRouterProvider
 */
function todoRoutes($stateProvider, $urlRouterProvider) { ... }

gives this output with ng-annotate:

/**
 * <p>Routing for the to-do app.</p>
 * @ngInject
 * @param {object} $StateProvider
 * @param {object} $urlRouterProvider
 */
todoRoutes.$inject = ["$stateProvider", "$urlRouterProvider"];
function todoRoutes($stateProvider, $urlRouterProvider) { ... }

Having said that, the preferred syntax for marking up a function for DI is nowadays using a directive prologue, like this: function todoRoutes($stateProvider, $urlRouterProvider) { "ngInject"; ... }. The reason for this is that other pre-processing tools may reorder or even remove comments while directive prologues will stay due to their semantic definition in the ES spec.

For using ng-annotate with ES6, we mark tag things up with "ngInject" as usual and run ng-annotate on the output from the transpiler so that is how your two last examples would be handled.

It is true (and documented) that ng-annotate does not currently take untranspiled ES6, TypeScript or any other compile-to-ES5-language as input. Implementing ES6 support fully is a somewhat large task because of ng-annotate's scope analysis, reference following, pattern matching and things like that. Implementing support for ES6 as input in the sense of just detecting explicit annotations (i.e. /*@ngInject*/ and "ngInject" is not an as large task, mostly updating the AST-walker.

How about this... I spend a little time thinking about how hard it would be to accept ES6 input in the case of explicit annotations, and you tweak your comparison to ng-annotate in the README to update the current state of things - deal? =)

[bholloway]

Sure. But before I write this up I'm just doing some tests to clarify exactly what the gap is.

Please let me know if you concur with the following.

1. Function literal with ES6 export syntax

Block comment - not supported (EDIT - originally thought this was ok but it is not).

/** @ngInject */
export default function foo(...injectables) {}

Inline comment - supported (when ng-annotate follows babel).

export default /** @ngInject */ function foo(...injectables) {}

String annotation - supported (when ng-annotate follows babel).

export default function foo(...injectables) { 'ngInject'; }

2. Return of function literal

Block comment - not supported.

/** @ngInject */
return function foo(...injectables) {}

Inline comment - supported (when ng-annotate follows babel).

return /** @ngInject */ function foo(...injectables) {}

String annotation - supported.

export default function foo(...injectables) { 'ngInject'; }

[bholloway]

Hmm.. I think I can swallow changing block comment syntax for returning function literals.

Particularly because my initial compile for a project I'm working on just went from ~80sec to ~60sec. That makes the difference between executive approval of Webpack and not!

I will need to take a look at our legacy code and see if it will cause any issues but with any luck not. Either way I think we can deprecate this loader and the corresponding browserify transform.

I was premature, the block comment in case 1 is not working.

I think the difference is that nginject-loader will look forward up to 3 statements in order to locate a function. Specifically to deal with the problems of compiled code.

[bholloway]

@olov you said that the string annotation is preferred for ES6.

It seems to me that this is a growing segment of the user base. There is a migration story from /**@ngInject*/ to 'ngInject'; as people adopt a compile stage.

In that case it is useful to make a tool that transforms /**@ngInject*/ to 'ngInject'; offline to the compile, with minimal disturbance to the code. Then change would then be permanent in the source code.

This could take the pressure off more comprehensive ES6 support for ng-annotate. Do you think there would be demand for this sort of tool?

[bholloway]

Ok got a branch with updated readme.md. Please take a look at #3 and let me know if I have missed anything.

[olov]

Just some background about comments: ng-annotate doesn't really have a notion of block and inline comment markup. The comment will bind to the function following it (if any). It does not matter if the comment is alone /* @ngInject */ or //@ngInject or part of a larger block /* yoyoma @ngInject foo */. So it's correct that an ng-annotate user would not do /*@ngInject*/ return function(a) { ... } but rather return /*@ngInject*/ function(a) { ... } or return function(a) { "ngInject"; ... }. In either case, exactly because comments are ambiguous, I came up with and now recommend the use of "ngInject" instead, because it is much clearer for the user, for ng-annotate and for other tools in the toolchain. I just think it's plain better.

A tool for converting /*@ngInject*/ to "ngInject"? I'd certainly recommend it! :)

[bholloway]

Cool.

I'll start by reimplementing this particular loader to convert /**@ngInject */ to 'ngInject'; with similar deprecate warnings as for my omit-tilde-webpack-plugin.

If I get anywhere with that I'll raise an issue on ng-annotate and ng-annotate-loader to get it documented.

[bholloway]

@olov I have completed this. Please refer to olov/ng-annotate#206.