Adding @ngInject where ng-annotate already matches should not create duplicate injection arrays

[gabrielmaldi]

Consider the following (which is real world production code):

var optionsView = function (route) {
    return {
        template: "<options items='items' on-select='go(item)' chevrons='true'></options>",
        //Annotated
        controller: function ($scope, $state, navigation) {
        }
    };
};

var rootViews = {
    "mainContainer_root.newsFeed.list": {
        templateUrl: "newsFeed/newsFeed.html",
        //Not annotated
        controller: function ($scope, navigation) {
        }
    },
    "mainContainer_root.settings": optionsView("root.settings")
};

$stateProvider
    .state("root", {
        abstract: true,
        url: "",
        views: rootViews
    });

As you can see, only the first controller: function () gets annotated. The other one doesn't because I'm using a variable instead of directly using $stateProvider (this is fine, I don't think ng-annotate should deal with variables).

However, this poses a problem because I need to /*@ngInject */ only the second controller: function (); if I also /*@ngInject */ the first one, it gets annotated twice. I don't want developers to be thinking whether they should include /*@ngInject */ in their code, so I run grunt-text-replace before grunt-ng-annotate to pre-process some files. With this particular example I have to distinguish between both kind of controller: function () uses to only /*@ngInject */ the second one.

Looking at ng-annotate-main.js I found this:

function matchDirectiveReturnObject(node) {
    // TODO make these more strict by checking that we're inside an angular module?

    // return { .. controller: function($scope, $timeout), ...}

    return node.type === "ReturnStatement" &&
        node.argument && node.argument.type === "ObjectExpression" &&
        matchProp("controller", node.argument.properties);
}

If that check were made, the first controller: function () wouldn't be annotated and I could just /*@ngInject */ every use of it in the file.

Or maybe /*@ngInject */ should only work on pieces of code which haven't already been annotated (which would also allow me to /*@ngInject */ every controller: function ()).

Thanks!

[olov]

There are two things going on here:

1. It is currently possible to trick ng-annotate to add duplicate injection arrays by putting /*@ngInject*/ where it isn't needed. This is what's causing your bug - will fix straight away. Sorry about this - I knew that this could happen (there's another TODO for it in the code) but forgot to address it.
2. ng-annotate matches a directive return object by the following pattern return { .. controller: function($scope, $timeout), ...}. It does so without checking any other context, most notably whether we're inside an angular module or not, with a risk of matching false positives (thus the TODO you found) edit: not true any longer, see next comment. Your optionsView.controller is getting annotated because if matches this pattern (even though it isn't a directive return object per se). If we would add checks to detect the pattern only when it is inside a .directive, then your controller would not get annotated any longer. I don't think that this is going to be necessary (checking that we're inside an angular module will most likely be fine to single out real false positives), but it's something to bear in mind and thus yeah I think it is a good idea for you to add the explicit annotation!

Thanks for filing, I'll release 0.9.4 once I have it fixed.

[gabrielmaldi]

Good job with the write-up and issue title, I know I struggled 😃

[olov]

Update on point 2 in my previous comment:
ng-annotate master now checks that we're inside an angular module for all matches and won't add/remove any annotation arrays otherwise (unless /* @ngInject */ annotated). This should reduce the risk of false positives dramatically. Your optionsView.controller will still get annotated because of the reasons already stated.

0.9.4 is going out tomorrow unless something unexpected happens.

[gabrielmaldi]

Excellent! Thanks for the great work.

One more question: how does grunt-ng-annotate update itself in relation to ng-annotate?

[olov]

Let me answer that: grunt-ng-annotate depends on any ng-annotate 0.9.x version ("^0.9.0") so there'll be no delay for releases before 1.0. Other ng-annotate integrations works the same generally. Once 1.0 is out we'll discuss and document what constitutes an ng-annotate major, minor and patch release in semver terms.

[mgol]

Yup, what @olov said. Note that if you add grunt-ng-annotate to dependencies and do 'npm install' and when after that a new 0.9.x ng-annotate version is released, invoking 'npm install' once again won't update ng-annotate, that's how npm works. If you need the new version, you have these options:

1. Remove grunt-ng-annotate (either via removing node_modules/grunt-ng-annotate manually or via invoking 'npm unlink grunt-ng-annotate' in the project directory) and invoke 'npm install'.
2. Use npm shrinkwrap to have dependencies versions specified even for sub-packages.

[gabrielmaldi]

Thanks guys!

[olov]

0.9.4 is now relased

[gabrielmaldi]

Just tested this, working great, thanks!