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.$bindTo implementation. #474

Closed

Conversation

jamestalmage
Copy link
Contributor

Just an idea, but it seems like it would be helpful to bind authentication state to scope.
This mimics the $bindTo() method used for data binding.

I haven't tested it at all, submitted solely for your consideration.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 4db2139 on jamestalmage:auth-bindto into 6ed1fc1 on firebase:master.

Users should be allowed to bind to a deep property
(i.e. 'obj.authData' instead of just 'authData').
Should be easily accomplished with $parse service
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling ef23918 on jamestalmage:auth-bindto into 6ed1fc1 on firebase:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 092da01 on jamestalmage:auth-bindto into 6ed1fc1 on firebase:master.

@jamestalmage
Copy link
Contributor Author

I added an example for how I envision the $bindTo function being used, and an associated build step.
It seems you guys are keeping examples elsewhere, but for right now, it does server as a sort of manual test. (It seems the old manual tests are broken?).

grunt example:auth

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 9b3eb19 on jamestalmage:auth-bindto into 6ed1fc1 on firebase:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 9b3eb19 on jamestalmage:auth-bindto into 6ed1fc1 on firebase:master.

Previously redirects were causing the reloaded page to default back
to popup mode. This solves that by saving the current choice in the url.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 0403260 on jamestalmage:auth-bindto into 6ed1fc1 on firebase:master.

@jwngr
Copy link

jwngr commented Nov 26, 2014

Prior to the 0.9.0 release, we provided an auth variable which we kept up-to-date with the current authentication state. I pushed to remove this in 0.9.0 since we now have the synchronous $getAuth() method. I do see the benefit of what you are doing here, but I just don't know if it provides anything you can't already do by calling $getAuth(). I also think if we re-introduce this kind of thing back in, it should just be as an auth variable like it used to be instead of having to register it yourself via $bindTo() (although I do like how you made the corollary there!).

@katowulf - what are your thoughts here?

@katowulf
Copy link
Contributor

I'd prefer to see this as a service and not part of the core. I think both of you have made great points here and in our technical discussions and I still agree with our decision to take this out.

@jamestalmage
Copy link
Contributor Author

The advantage of $bindTo() over $getAuth() or even an auth variable is that $bindTo() will automatically schedule a $digest cycle whenever login status changes. Users could do $watch(auth.getAuth(), function(){....}), but that doesn't handle triggering a $digest.

Users could certainly implement their own version of this pretty easily.

ref.onAuth(function(authData){
  scope.$apply(function(){
    scope.auth = authData
  });
});

The potential downfall there however, is that the Firebase reference is long lived and is now holding a reference to scope, so that scope can't be garbage collected. You could see how this could get messy inside a paginated ng-repeat. Of course, there are easy ways around that, but it's now complicated enough that it seems like something we should just solve for the users (especially since it is so easy to do so).

I'm curious why you argued to remove the auth variable. Maybe there is a good argument against what I am trying here that I'm just not understanding.

@katowulf
Copy link
Contributor

Didn't realize that $onAuth() isn't triggering a digest. That we should definitely fix, which should make the rest of this much simpler. The reason auth was decided against is that it was essentially creating a global variable and all the baggage that comes with that (additionally there was no way to tell if it changed unless one $watched it, which you've already noted the issues with).

So essentially, $onAuth() should work in a real-time, event-driven manner, vs the old auth variable's essentially static behavior.

@jamestalmage
Copy link
Contributor Author

Here is the use case I am trying to enable.

Controller:

ref.$bindTo($scope,'authData');

HTML:

<button ... ng-hide="authData.provider=='twitter'">Twitter</button>

<!-- or if you've implemented some additional info in your custom auth -->
<div ng-repeat="message in messages">
  <span>{{message.contents}}</span> 
  <button ng-show="authData.isModerator">Moderate this post...</button>
</div> 

@al-the-x
Copy link

al-the-x commented Dec 2, 2014

The current implementation also doesn't play nice with the controllerAs syntax, either. The $onAuth method really should run the digest loop if not running already.

@katowulf
Copy link
Contributor

katowulf commented Dec 2, 2014

@al-the-x please submit a new issue for that; will get it added; has nothing to do with Auth.$bindTo and won't get fixed here (as evidenced by the fact that this is the second comment added to this thread on that topic).

@jwngr
Copy link

jwngr commented Dec 17, 2014

Lots of good stuff in this thread. @katowulf went ahead updated $onAuth() to fire a digest loop so that issue is resolved. As for $bindTo() I think @katowulf and I are in agreement that this is not really needed and don't think the benefits outweigh the costs. With $onAuth() running a digest loop now, this should be super easy and natural for people to do anyway.

Closing this PR.

@jwngr jwngr closed this Dec 17, 2014
@al-the-x
Copy link

Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants