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

Injecting non-existent Services into Resolve should throw error. #508

Closed jdpedrie closed 10 years ago

jdpedrie commented 11 years ago
$stateProvider.state('myState', {
    url: '/data/:id,
    templateUrl: 'views/container.html',
    controller: 'FormCtrl',
    resolve: {
      Data: ['$tateParams', 'Model',
      function($stateParams, Model) {
        return Model.getDataById($stateParams.id);
      }]
    },
    data: {
      name: 'Details'
    }
  });

Given the above code, obviously $tateParams is a mistake. This should throw an error, or provide some sort of feedback, but instead it fails silently. The same is true with any non-existent service. Trying to inject any dependency, whether built in or part of the App, that doesn't exist, causes the route to not load, but does not give any feedback as to what the problem is.

randallmeeker commented 11 years ago

I ran into this, but it was any error in the resolve makes in silently fail.

nateabele commented 11 years ago

So, when this happens, does Data return a rejected promise?

randallmeeker commented 11 years ago

In my case nothing is returned. The controller does not initialize and the template does not render. The script simply halts at the resolve unless you fix your typo/error. Makes for interesting debugging. The following plunkr edits route 2 so you can see http://plnkr.co/edit/SLjrs5?p=preview

daspecster commented 11 years ago

+1!

randallmeeker commented 11 years ago

I actually just encountered a related issue. I made a factory that had a bad injected service and in using that factory in my resolve I also had the silent fail. The good news is that the silent fail is such a unique error that I now recognize it and can debug.

jdpedrie commented 11 years ago

In the packaged file, angular-ui-router/release/angular-ui-router.js, I added a log to alert me when something is broken:

       function proceed() {
          if (isDefined(result.$$failure)) {
            // Log errors that would otherwise result in a silent failure.
            console.log('fail!', result.$$failure, key);
            return;
          }
          try {
            invocation.resolve($injector.invoke(invocable, self, values));
            invocation.promise.then(function (result) {
              values[key] = result;
              done();
            }, onfailure);
          } catch (e) {
            onfailure(e);
          }
        }

Obviously this isn't a production fix, but it's useful for debugging whilst a permanent fix is in the works.

jeme commented 11 years ago

The error is actually not entirely silent, but the problem is that it happens in a promise and possibly in an async context, so you need to listen to $stateChangeError to capture it... E.g. as in: http://plnkr.co/edit/ioJnJO?p=preview

In that case I just create a page wide controller, but there are other ways to attach to it ofc... but that was just to show that a meaning full error is actually given (Unknown provider)

@jdpedrie the above demonstration would probably serve you better as you then don't have to modify the source on each update.

santyclaz commented 11 years ago

Hmmm.. Would the solution not be to remove the try/catch in the proceed function?

function proceed() {
    if (isDefined(result.$$failure)) return;
    try {
        invocation.resolve($injector.invoke(invocable, self, values));
        invocation.promise.then(function (result) {
            values[key] = result;
            done();
        }, onfailure);
    } catch (e) {
        onfailure(e);
    }
}

Glancing at this snippet of code, it seems like it is doing more than it should, catching all errors and handling it for us. Is there a specific reasons it is there, instead of letting users of ui-router handle the errors themselves? It makes it difficult to debug an application when it fails and no errors show up in the debugger console.

Burgov commented 10 years ago

This is really annoying, especially when you have resolves depending on each other. I can become a hell to figure out what is going wrong since you don't see any error messages

nateabele commented 10 years ago

Hmmm.. Would the solution not be to remove the try/catch in the proceed function?

@santyclaz I don't think the solution is to remove it, but I could certainly understand making it more targeted, so that it only catches errors from within the resolves, and not from the definition of the resolves themselves.

nateabele commented 10 years ago

Alternatively, something like this might work, just above the try block:

forEach($injector.annotate(invocable), function(name) {
  if (!$injector.has(name) && !values[name]) throw new Error(...);
}

...or something.

santyclaz commented 10 years ago

@nateabele hmm.. so it seems throwing an error within a promise is equivalent rejecting a promise?

So if I have something like this plunker:

...
resolve: {
  testResolve : function() {
    console.log("testResolve");
    throw new Error("RUH ROH!");
    return 'test';
  }
},
...

the throw new Error("RUH ROH") doesn't print anything to the console, and instead just rejects the promise.

I can see the argument for why it should be like this, but I feel like I would prefer a thrown exception to just behave as it would by default; and if I don't want that behaviour, just define a try-catch myself and reject via the $q.reject() API (or maybe configure the angular $exceptionHandler, though I haven't looked into that in depth to know how that would work).

nateabele commented 10 years ago

@santyclaz Normally I try to be accommodating about different approaches, but the resolve system is designed around promises, and promises are implemented according to a spec, so feelings and preferences don't enter into the equation.

Having said that, the original issue was about injecting services, which is a configuration-time thing, not a runtime thing, so that should be caught (with an exception) before any state transitions are ever even initiated.

santyclaz commented 10 years ago

@nateabele ah thanks for referencing the spec, I didn't realize there was one. Thanks!

isakb commented 10 years ago

@jeme in the JS file at that link I only see "// Code goes here".

Is it just my feeling, or is plunker links pretty broken in general all over the web?

jeme commented 10 years ago

@isakb I don't have any issue with them, so I don't know?

Here is an extract of the important stuff:

  <script>
    var myapp = angular.module('myapp', ["ui.router"])
    myapp.config(function($stateProvider, $urlRouterProvider){

      // For any unmatched url, send to /route1
      $urlRouterProvider.otherwise("/route1")

      $stateProvider
        .state('route1', {
            url: "/route1",
            templateUrl: "route1.html"
        })

    myapp.controller('site', function($scope) {
      $scope.$on('$stateChangeError', function(evt, to, toParams, from, fromParams, error) {
        console.log("WHOOAAAATTT!!! " + error);

      })

    })
  </script>
isakb commented 10 years ago

Thanks @jeme, I think it might be just on my computer, I tried the link from a different computer now and it worked better. :)

timkindberg commented 10 years ago

@nateabele can you confirm if this is a dupe of #469?

nateabele commented 10 years ago

@timkindberg Looks that way, yeah.