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

Auth promises and callbacks #456

Closed
kingcody opened this issue Aug 16, 2014 · 10 comments
Closed

Auth promises and callbacks #456

kingcody opened this issue Aug 16, 2014 · 10 comments

Comments

@kingcody
Copy link
Member

Looking over client/components/auth/auth.service it seems that it would be beneficial for us to use a signature for the methods that would allow for synchronous and async callback/promises.

I would propose:

function(callback, safe) {
  if (arguments.length === 0) {
    return synchronous.result;
  } else {
    var deferred = $q.defer();
    async.then(function(data) {
      deferred.resolve(data);
      callback(data);
    }, function() {
      (safe) ? deferred.resolve(null) : deferred.reject();
      callback(null);
    });
    return deferred.promise;
  }
}

Its not terribly complicated and I feel provides maximum usability. Such as, all current calls to the auth methods would be maintained sync, we could remove methods such as isLoggedInAsync, and also we could use the promises they return in resolves for routes which could seriously improve the routing authentication.

I'd also like to point out the use of the 2nd argument safe, I've used it to allow us to receive promises that only resolve and not reject. This could be useful in a route resolve as well, being that if a promise is rejected then $routeChangeError or $stateChangeError is fired and we may not always want that.

Another advantage would be that you could chain the promises with finer grained control, dictating whether or not a promise resolves into another (even on error) or is rejected and the chain canceled.

@kingcody
Copy link
Member Author

With the release of angular 1.3.0 (still beta atm), $q receives some notable upgrades that would allow us to simplify the process even more.

@kingcody
Copy link
Member Author

After more investigation, I think that resolves wouldn't (at this time) be suited for authentication in routes. However is still feel that returning promises from the Auth methods would still be beneficial.

Examples:

_Auth.getCurrentUser_

getCurrentUser: function(cb) {
  if (arguments.length === 0) {
    return currentUser;
  } else {
    var value = (currentUser.hasOwnProperty('$promise')) ? currentUser.$promise : currentUser;
    cb = safeCb(cb);
    return $q.when(value)
      .then(function(user) {
        cb(user);
        return user;
      })
      .catch(function() {
        cb({});
        return {};
      });
  }
}

_Auth.isLoggedIn_

isLoggedIn: function(cb) {
  if (arguments.length === 0) {
    return currentUser.hasOwnProperty('role');
  } else {
    cb = safeCb(cb);
    return this.getCurrentUser(null)
      .then(function(user) {
        var is = user.hasOwnProperty('role');
        cb(is);
        return is;
      });
  }
}

_Auth.isAdmin_

isAdmin: function(cb) {
  if (arguments.length === 0) {
    return currentUser.role === 'admin';
  } else {
    cb = safeCb(cb);
    return this.getCurrentUser(null)
      .then(function(user) {
        var is = user.role === 'admin';
        cb(is);
        return is;
      });
  }
}

Still allows for use as route resolves, and synchronous code by not passing any arguments.

Just some 'opinions'...

@DaftMonk
Copy link
Member

That looks good. Much cleaner.

@kingcody
Copy link
Member Author

@DaftMonk you can checkout this branch if you'd like to see my changes.

kingcody added a commit to kingcody/generator-angular-fullstack that referenced this issue Aug 20, 2014
Changes:
- getCurrentUser, isLoggedIn, and isAdmin are now sync if no arg and async with an arg
- Use Error first callback signature where applicable
- Remove unused arguments from Auth service
- Remove isLoggedInAsync
- Switch use of isLoggedInAsync to isLoggedIn
- Add/Improve comments
- Fix client/app/account(auth)/settings/settings.controller(js).js

Breaking Changes:
- Callbacks that return Errors, use 'Error first' signature

Closes angular-fullstack#456
@sparqueur
Copy link

Hi,
Just a little question about the way access the currentUser in a controler safely.
I currently load it in my controler this way :

alert ( Auth.getCurrentUser().name );

Unfortunatly sometimes retrieving the user (when refreshing page) takes too much time --> undefined

Workaround for me :

User.get().$promise.then( function(user) {
    alert ( user.name );
}

I would prefer accessing currentUser through Auth service (but perhaps I'm just wrong).

Note : I am just trying to enhance my user "settings" page

Thanks in advance

@kingcody
Copy link
Member Author

kingcody commented Sep 8, 2014

@sparqueur check out #465. In auth.service.js the method getCurrentUser is written to be synchronous/asynchronous. When used asynchronously it should provide you with the user object after its resolved.

@sparqueur
Copy link

Ho, sorry. Think I was a little tired...
Thanks for this cool generator which made me win a lot of time.

@kingcody
Copy link
Member Author

kingcody commented Sep 8, 2014

@sparqueur no problem. Hope that helps you out 😄

DaftMonk pushed a commit that referenced this issue Sep 17, 2014
Changes:
- getCurrentUser, isLoggedIn, and isAdmin are now sync if no arg and async with an arg
- Use Error first callback signature where applicable
- Remove unused arguments from Auth service
- Remove isLoggedInAsync
- Switch use of isLoggedInAsync to isLoggedIn
- Add/Improve comments
- Fix client/app/account(auth)/settings/settings.controller(js).js

Breaking Changes:
- Callbacks that return Errors, use 'Error first' signature

Closes #456
@kingcody
Copy link
Member Author

kingcody commented Oct 1, 2014

Closed by #465

@kingcody kingcody closed this as completed Oct 1, 2014
@ghost
Copy link

ghost commented Nov 7, 2016

@sparqueur thanks a lot, you saved my day!

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

4 participants