angular-ui / ui-router

The de-facto solution to flexible routing with nested views in AngularJS
http://ui-router.github.io/
MIT License
13.53k stars 3k forks source link

Throw Error when `resolve` does not find dependency to inject #469

Closed frapontillo closed 11 years ago

frapontillo commented 11 years ago

Say I have a factory called Config and there is a state such as:

$stateProvider.state('app', {
  url: '/',
  templateUrl: 'views/main.html',
  controller: 'MainCtrl',
  resolve: {
    config: function(Config) {
      return Config.reloadConfig();
    }
  }
}):

The resolve function of course fails, but no Error is thrown! When the project is minified (which is the only case when you notice something is wrong) finding this error is a pain; I had to go by trial and error through a lot of files to notice this.

nateabele commented 11 years ago

I don't understand. Why does it "of course" fail? Because the code is minified and Config is renamed? Try listening for $stateChangeError, or using the error callback in the result of transitionTo().

frapontillo commented 11 years ago

Yes, because it's minified. If I don't correctly declare dependencies in a controller, an error is thrown so I can understand where the error is and how to fix it. It'd be a mess if things start failing silently.

I think that the resolve function is evaluated somewhere in a try block, but the error isn't properly handled, as the console does not display it as it should.

nateabele commented 11 years ago

The error is propagated through the error callback of the promise returned by transitionTo(). Throwing an error would break the model.

frapontillo commented 11 years ago

Maybe printing a message on the error stream would be considered acceptable? Just my 2 cents.

laurelnaiad commented 11 years ago

When you work with promises in angular, it would behoove you to handle the error callbacks in every case. And in such a callback you can log the error yourself. I for one find it annoying when libraries decide on my behalf that they will print something to the log.

edit: if that's what you meant by 'error stream', but now that I'm rereading I'm not sure if that's what you meant.

nateabele commented 11 years ago

@frapontillo You should be able to achieve that for all promise rejections with $provide.decorator().

frapontillo commented 11 years ago

The error callback is one thing, a dependency error is another one. For example, the $http and $resource services do throw an error when there is a network one (say 404). And they do return a $promise.

@stu-salsbury yep, I meant the console.error stream :)

laurelnaiad commented 11 years ago

Funny -- you mention the exact console error that drives me nuts!

ratbeard commented 10 years ago

I had a typo in one of my Services I was using inside of the resolve block and was bit by this as well. I like @frapontillo's suggestion of adding a console.error in, around here I think:

https://github.com/angular-ui/ui-router/blob/aa0355567b60b2b07dadc5efa42cc4c212b99f2b/release/angular-ui-router.js#L371

I would say the api and concepts in this library are quite large and take time to fully grasp. Most developers probably starting like me with just a few states and grow it from there, not really thinking of handling errors until they have to spend an hour tracking down a silent failure. Duplicate tickets seems to indicate this isn't too uncommon.

timkindberg commented 10 years ago

I'd be in favor of a console.error entry for broken resolve.

nateabele commented 10 years ago

The resolve just needs to be checked at initialization time.

reebalazs commented 10 years ago

Hi,

I must admit I don't understand the resolution of the issue. Could you please help me understand the status?

My original problem is that any exceptions in the resolver function are caught silently, and when any such exception happens it causes my code breaking silently. We investigated some misterious hangs which left no sign of error. Now I realize that the silent exceptions are the culprit and this is how I found #508 and #696 that are now marked dupes of this issue, which is why I ask here.

@nateabele, you mention: "The error is propagated through the error callback of the promise returned by transitionTo(). Throwing an error would break the model."

When an exception happens in my code in the resolver function, | want to see it in the console. Also, ... yes, it's broken. It will not work anyway since execution stopped at the point of the exception. There is nothing ui-router can do for me at this point.

Could you please help me understand (or point to docs or code) why throwing an error would necessarily break the model? I think this should not be necessary, and perhaps the code can be improved to avoid an exception all-catch which is in general, a very bad thing.

nilskp commented 10 years ago

I have the same problem. My controller fails to initialize after I added resolve and I have no idea why, because the console is silent.

nilskp commented 10 years ago

By wrapping every resolve function in try/catch with console logging in the catch, I get the expected error. But why isn't this automatic?

eddiemonge commented 10 years ago

It should throw an error for missing dependency like so:

  $stateProvider.state('main', {
    resolve: { myResolve: function (doesNotExist) {} }
  });
Error: [$injector:unpr] Unknown provider: doesNotExistProvider <- doesNotExist
patrick-fls commented 10 years ago

@nilskp can you provide an example please? the lack of error catching is driving me nuts!

nilskp commented 10 years ago

Something like this (but that obviously won't work for unresolved dependencies):

$stateProvider.state('main', {
  resolve: {
    myResolve: function() {
      try {
        // Do stuff
      } catch (err) {
        return console.log(err);
      }
    }
  }
});
eddiemonge commented 10 years ago

It does seem silly to have to add an event listener to catch errors that angular should obviously be catching.

$rootScope.$on( '$stateChangeError', function (event, toState, toParams, fromState, fromParams, error) {
  console.log( 'Resolve Error: ', error);
});

^^^ that is a lot of code just to catch errors that should be thrown

nicovalencia commented 10 years ago

I'm bumping this as another user that spent way too long hunting down a silently swallowed error. I understand that logging level is a bit controversial, but this is an error... maybe an in-between is tracking unresolved loops and logging/throwing the caught error after the timeout -- right now it just hangs indefinitely.

Thoughts?

:strawberry:

nicovalencia commented 10 years ago

Also :+1: for catching errors by listening to the error event, however I'd agree that sometimes the development process doesn't always prioritize these things until later on. If ui-router's opinion is that handling errors like this is a first priority, maybe this strategy should live near the beginning of the documentation, so everyone gets setup with the error catcher before they hunt down any ghosts?

:cake:

aaronyo commented 10 years ago

Why isn't there a default handler for $stateChangeError that rethrows the error? Is that considered a more annoying default than swallowing?

ratbeard commented 10 years ago

@aaronyo since $stateChangeError is using angular's $broadcast mechanism, the event emiter and event listener are decoupled so ui-router doesn't know if the error was handled or not. Well you could iterate through the DOM, looking at the internal $$listeners properties on scopes... [1]

If there was an api where ui-state was aware of the listener registration, or if it used direct registration like stateProvider.onError = function () { ... } ) then it could know if you were handling the error or not.

I agree with @nicovalencia that if the lack of error message can't be fixed, it should be featured more prominently in the docs, where resolves are introduced.

[1]

$('.ng-scope').filter(function () { return $(this).scope().$$listeners['$stateChangeError'] } ).length
mallowigi commented 10 years ago

+1 for @ratbeard suggestion

booybooy commented 9 years ago

Just got bit by this while refactoring from ngRouter to uiRouter. Spent a good chunk of my morning trying to figure why the page is not loading while no errors are showing on console. So +1 for @nicovalencia suggestion.

nicops commented 9 years ago

A console.error never hurt anybody and can be easily done. People could find it annoying because the error was actually expected and was actually catched in some cases? Ok, but that annoyance is far less a deal than an error being silently swallowed! If figuring out if the error was being captured or not is complicated, there could be just a config variable to disable the logging in such cases.

Another thing: stateChangeError only works in transitions! but what if it's the first state you're entering to?

AlicanC commented 9 years ago
var app = angular.module('myApp', []);

app.run(['$rootScope', function ($rootScope) {

    $rootScope.$on('$stateChangeError', function (event, toState, toParams, fromState, fromParams, error) {

        throw error;

    });

}]);
demisx commented 9 years ago

@AlicanC :+1: This is a must for any app using ui router.

a2f0 commented 9 years ago

@AlicianC that's the jam right there. Thanks buddy.

OndeVai commented 9 years ago

Please, log a console error! I don't care about esoteric arguments regarding promises, this has wasted the time of MANY developers and needs to be addressed!

nateabele commented 9 years ago

@OndeVai $rootScope.$on("$stateChangeError", console.log.bind(console));

blittle commented 9 years ago

It is a cardinal sin for a library to swallow user errors! Expecting the user to need to try {} catch or Promise.catch the error assumes the user knows that there is an error in the first place.

nateabele commented 9 years ago

Show me a promise-based library that surfaces any kind of errors outside of promise rejection.

blittle commented 9 years ago

@nateabele RX.js (reactive extensions) will surface an error if there is no handler to accept it.

nateabele commented 9 years ago

Rx.js is observable-based, which has the benefit of being explicit about managing handlers and flow as first-class things. I can't think of a way to detect whether a $stateChangeError handler is registered. The hooks system in 1.0 is a little different, so we could possibly address it then.

AlicanC commented 9 years ago

People are having this problem since 2013. You either have to fix your library or fix your documentation.

Throwing an error might not be a solution but putting a big fat red box in the documentation which warns users about this is a solution.

Also, does $rootScope.$on("$stateChangeError", console.log.bind(console)); even work if you put it in your state controller? I think this should be in app.run() and if that is the case, that should also be specified.

eddiemonge commented 9 years ago

its documented at http://angular-ui.github.io/ui-router/site/#/api/ui.router.state.$state#events they could be more clear though.

nateabele commented 9 years ago

You either have to fix your library or fix your documentation.

I'm open to suggestions on how to do the latter.

windmaomao commented 9 years ago

It'll be highly recommend to check $stateChangeError by default, maybe we could just add a listener in the library ?

nateabele commented 9 years ago

Yes, 1.0 will have a default error handler.

nonplus commented 8 years ago

A missing dependency in a state resolve definition is a programming mistake and the library should help us catch these. This is a different from a resolve that fails (returns a rejected promise) which is a run-time error.

I would expect these mistakes to be logged out of the box (and possibly add a config option to suppress this logging for hacks that prefer to ignore errors). For now, I'm forced to do the following in my apps:

$rootScope.$on( '$stateChangeError', function (event, toState, toParams, fromState, fromParams, error) {
    if (error.message.match(/\[\$injector:unpr\]/)) {
        // Log missing 'resolve' dependency
        // See: https://github.com/angular-ui/ui-router/issues/469
        console.error("Invalid resolve in state '" + toState.name + "' - " + error.message);
    }
});
nonplus commented 8 years ago

I've published my workaround as https://github.com/nonplus/angular-ui-router-resolve-error.

Although I'd still prefer/expect this behavior to be built in...

christopherthielen commented 8 years ago

See https://ui-router.github.io/docs/latest/classes/state.stateservice.html#defaulterrorhandler in 1.0

moneytree-doug commented 8 years ago

Just wanted to say that this happens still, and that I support a functionality that throws

AldoMX commented 8 years ago

@nonplus 's workaround was still failing silently, so I simply did the right thing to do: explode at any error, no mercy.

(function (angular) {
    'use strict';

    angular
        .module('YOUR-MODULE-NAME')
        .run(throwErrors);

    throwErrors.$inject = ['$rootScope'];
    function throwErrors($rootScope) {
        $rootScope.$on('$stateChangeError', function (event, toState, toParams, fromState, fromParams, error) {
            throw error;
        });
    }
})(window.angular);
moneytree-doug commented 8 years ago

@AldoMX That's the correct expected behaviour

peterbraden-amana commented 7 years ago

This is a horrible experience - I spent days trying to find an error in an unrelated piece of code because an injection error was swallowed. AldoMX's fix should be the default behaviour.