Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed requests should throw an error, not just return it #69

Closed
kentcdodds opened this issue May 30, 2014 · 7 comments
Closed

Failed requests should throw an error, not just return it #69

kentcdodds opened this issue May 30, 2014 · 7 comments
Assignees

Comments

@kentcdodds
Copy link
Contributor

On this line in findAll, when the request fails (404 for example) angular-data is returning the error. This causes a resolve in a route to resolve to the error. Because of this when I do this, my injected users is the err object that angular-data is returning (not great...)

// ... some state definitions

$stateProvider.state('root.auth.home.locks.list', {
  url: '',
  templateUrl: homeTemplateRoot + 'Locks/LocksCtrl.html',
  controller: 'LocksCtrl',
  resolve: {
    locks: resolveDS('findAll', ['lock', {}]),
    users: resolveDS('findAll', ['user', {}])
  }
});

// ... other state definitions

function resolveDS(method, args) {
  if (!_.isArray(args)) {
    args = [args];
  }
  return function(DS) {
    return DS[method].apply(DS, args);
  };
}

// in my LocksCtrl
angular.module('pk.web').controller('LocksCtrl', function ($scope, locks, users, _) {
  // some $scope initialization stuff using the injected users.
  // however, if the request to findAll users fails, the users object is actually the error returned by angular-data. I would rather the controller not even initialized if this happens.
});

Let me know if I need to be more clear on what is actually happening. But I'm pretty sure all that needs to happen is on that line we need to change the "return" to "throw"

@kentcdodds
Copy link
Contributor Author

For more info, checkout propagation from the q library.

@jmdobry
Copy link
Member

jmdobry commented May 30, 2014

I found where Angular's $q documentation mentions you have to rethrow errors to propagate them. https://docs.angularjs.org/api/ng/service/$q#reject

Their example uses $q.reject(reason) though, I'm wondering how long that has been in the API.

@jmdobry
Copy link
Member

jmdobry commented May 30, 2014

Since 1.0.2

@jmdobry
Copy link
Member

jmdobry commented May 30, 2014

There are a five or six places that need to be changed from return err; to return _this.$q.reject(err); or return $q.reject(err);. Easy fix.

@kentcdodds
Copy link
Contributor Author

So, why can't we just rethrow the error?

@jmdobry
Copy link
Member

jmdobry commented May 30, 2014

I imagine $q.reject is faster than throw.

@kentcdodds
Copy link
Contributor Author

Ok, cool

jmdobry added a commit that referenced this issue May 30, 2014
@jmdobry jmdobry self-assigned this May 30, 2014
@jmdobry jmdobry closed this as completed May 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants