Warn on injection by name that will fail when minified

[CyborgMaster]

It would be awesome to be able to set some sort of configuration to cause angular to throw warnings (or errors) when it is injecting by function argument name.

I'm working on a project that has it's javascript minified in production and therefore need to specify injection arguments explicitly using either the array syntax:

["$scope", function($scope) { ... }]

or the $inject syntax:

controllerFunction.$inject = ["$scope"]

I'm using those (almost) everywhere in my project, but one of them snuck by me into the minified javascript and was a huge pain to find. I just got errors like

Error: [$injector:unpr] Unknown provider: eProvider <- e

I did eventually find it, but only by trial and error and a lot of searching through the code.

I've modified the angular source a bit to help me find these down the road. I've modified the annotate function as follows:

function annotate(fn) {
  var $inject,
      fnText,
      argDecl,
      last;

  if (typeof fn == 'function') {
    if (!($inject = fn.$inject)) {
      $inject = [];
      if (fn.length) {
        fnText = fn.toString().replace(STRIP_COMMENTS, '');
        throw 'Using injection by name, should explicitly define ' +
          'requirements using $inject or an array! Function text:' +  fnText;
        argDecl = fnText.match(FN_ARGS);
        forEach(argDecl[1].split(FN_ARG_SPLIT), function(arg){
          arg.replace(FN_ARG, function(all, underscore, name){
            $inject.push(name);
          });
        });
      }
      fn.$inject = $inject;
    }
  } else if (isArray(fn)) {
    last = fn.length - 1;
    assertArgFn(fn[last], 'fn');
    $inject = fn.slice(0, last);
  } else {
    assertArgFn(fn, 'fn', true);
  }
  return $inject;
}

That way I am warned in development mode if my code won't work when minified. I know this isn't the right way to do it because it prevents injection by name from working at all (which is why this isn't a pull request), but maybe there is a way we can figure out to make this a configurable option.

Or if we can't maybe angular should always throw a warning (not exception) because shouldn't we all be planning to minify our javascript in production?

[caitp]

Something like this wouldn't be too bad, but IMO it would have to be a custom build of angular --- The "primary" build would need to remove it via a preprocessor or something.

We often like to use tools like ngmin in order to use minify-safe DI notation, but use the simple notation for readability. Tests will often use the pre-ngmin'd code, and you don't really want your test runner being spammed with "WARNING: BADNESS -- USE SAFER DI NOTATION".

I guess what I'm saying is, it should ideally be an opt-in warning, and preferably one which wouldn't add unused bytes to the default build (although it probably wouldn't be that big of a deal).

[CyborgMaster]

It definitely would have to be opt in. I actually have those lines commented out most of the time because they do spam my tests with warnings (I write my tests using the name based injection), but then I turn it on and check things before pushing to production. It would work with 2 builds of angular, one I could require for tests, and the other I could require for dev/production. Similar to the way I use angular-mocks to help stub things during tests.

+1 This would help so much in dev

[IgorMinar]

I agree that we should do this. but we need to figure out how to do it so that we don't require strict token declaration for test code and prototyping code.

in short, I think we should have an option that would enable/disable DI token inference via argument names.

When strict token annotation mode is of, the argument name inference will be disabled, but we should then change the inject mock helper method to wrap all functions passed in into ['token1', 'token2', fn] array. Where token1 and token2 was parsed out of fn.toString() - just as we currently do by default. This would allow us to have a strict token identification when we want it, but convenient inference in tests and prototype apps where convenience matters more than minification.

This would allow us to complain if we see any of this in production code:

function MyController($location) { // missess ['$location'] annotation
  ...
}

['$location', '$http', function MyController($location, $http, someOtherService) { // misses 'someOtherService' annotation
}]

['$location', '$http', function MyController($location) { // misses $http argument
}]

but not complain if the code above is mixed with a test code like this:

it('should do something awesome', inject(function($location) { // ['$location'] annotation intentionally ommited
});

[IgorMinar]

Anybody wants to work on this? it shouldn't be that hard. we just need to figure out the configuration api and what the default should be.

[timothyleerussell]

It would be nice if all of the examples just assumed that minification would be taking place at some point as the error messages for this problem are pretty obtuse.

It had me stymied for a couple of hours. Everything worked perfectly locally (because it wasn't being minified) -- however once I put it into my test environment on Azure it gave me the white screen of death. Once I figured out that it was the minification process that was causing dependency injection to fail, I went back through all my code and used the array syntax. Bravo -- but then it still didn't work.

Then the fine tooth comb came out and I found one pesky little omitted array after a couple of hours.

app.config(['$provide', function ($provide) {
$provide.decorator('$log', function ($delegate, $sniffer) {

instead of:

app.config(['$provide', function ($provide) {
$provide.decorator('$log', ['$delegate', '$sniffer', function ($delegate, $sniffer) {

See how I thoughtfully changed it on the line directly above while simultaneously forgetting to make the same change on the line below? Dammit.

That said, I learned a lot about Angular while figuring it out -- three cheers for user error.

Better error messages are always good, of course.

[caitp]

Good news, we'll have a fix for this shortly.

[mikehaas763]

Being that this is opt-in by using the ngStrictDi directive, Is there anything stopping us from backporting this to the 1.2.x line?

[caitp]

it was initially backported, and I had to revert it because it was too featureful :( only bugfixes in 1.2* I'm afraid.

[davegurnell]

@caitp is that the final word on Angular 1.2 support?

I'd leap at the opportunity to use this feature -- I've wasted hours on bugs caused by DI annotations.

I can't update to Angular 1.3 because of the lack of IE8 support.

[caitp]

you could always cherrypick that feature into 1.2 manually, but yeah we can't include it in a release unfortunately