FirebaseExtended / angularfire

AngularJS bindings for Firebase
MIT License
2.73k stars 632 forks source link

Add support for new Firebase 3.x.x. authentication methods #764

Closed jwngr closed 3 years ago

jwngr commented 8 years ago

There are a bunch of new authentication methods in the Firebase 3.x.x SDK which are not found in AngularFire 2.x.x. These include the following:

The only reason we include the existing set of authentication methods is because those are the ones that were around in the Firebase 2.x.x SDK. Now that we have a bunch of new methods, we should probably include them in the $firebaseAuth service or else users will have to drop down to the vanilla Firebase SDK half of the time.

The counterpoint to adding them is that we end up just re-implementing a bunch of existing functionality in the vanilla SDK. Is there a different approach we can take to handle the $digest loop issues that doesn't involve use wrapping every method in the underlying SDK?

cc/ @katowulf, @abehaskins, @davideast - Thoughts?

jwngr commented 8 years ago

We should also consider deprecating $getAuth() in favor of $getCurrentUser().

davideast commented 8 years ago

Is there a different approach we can take to handle the $digest loop issues that doesn't involve use wrapping every method in the underlying SDK?

Unfortunately not, any async event will need to be wrapped.

I think it's important to provide these methods to keep continuity for the developer, so I'm on board 💯.

katowulf commented 8 years ago

Well, technically we have lots of terrible choices available!

We could monitor web sockets and trigger a compile when an incoming event is received (Angular 2 does some things like this internally; this one is actually not "terrible"). We could $interval( $timeout.bind(null, function(){}), 500 ) to trigger an automatic compile every half of a second. We could hack into the Firebase notification process and make it trigger $timeout.

And also not a completely terrible idea, although not a perfect one: We could just provide a general wrapper that allows you to call pretty much anything that returns a promise and converts it to an Angular promise which trigger a compile. Thus you could do something like: angularfire.when( firebase.auth().signInWithPassword() ).then(...);

For that matter, we could just provide some instructions in the readme for "calling other SDK methods" which uses $q.when( ... ) to achieve the same result.

abeisgoat commented 8 years ago

I'm not actually sure that promise idea would work Kato.

I'm pretty sure (based on my experience upgrading the tests) that Promises resolving don't trigger digestion, in fact it's the opposite - .then() calls wont be called until a digestion happens, so wrapping with $q.when would result in weird / inconsistent response times since the promises wouldn't resolve until something else triggered a digestion. (I think)

katowulf commented 8 years ago

I'm fairly sure that's only true of the mock promises, or maybe has to do with the fact that we have multiple layers of $timeout and promises, which requires multiple flushes of the mock $q library. I'm not sure why they behave so strangely. But $q does trigger an eval on root scope.

the thing with $q is that when your promise is resolved (and also presumably on reject or notify), it invokes your handlers within $rootScope.evalAsync which ensures that after invocation, it'll trigger a digest and thus the rest of your application can have a chance to update or otherwise respond to the changes, just the way we like it.

This is also in the docs but too dense to link to and summarize quickly, so this non-authoritative answer is a better tl;dr.

But even if they don't trigger a compile (it's not necessarily a digest loop, btw) then a simple wrapper achieves the same end.

ghost commented 8 years ago

Having issues using vanilla 3.0 authorization in AngularJS, any eta on having 3.0 auth support?

katowulf commented 8 years ago

@CTucker1327 this issue is about peripheral methods supported in the new auth SDK and not about vanilla auth. Auth is supported.

Please submit a new issue rather than responding to a tangentially related thread. Be sure to include an mcve and any useful debug output from firebase.database.enableLogging(true); and error logs.

idanen commented 7 years ago

Is someone working on implementing the methods @jwngr mentioned (didn't see any PRs)? Do you need some help on those (maybe add label 'help-needed')? Can you update on the progress? Anyway, would love to help, just don't want to do work that someone is already doing

jwngr commented 7 years ago

Nope, no one is working on this. But we would gladly enjoy some help and will take some PRs if you are willing to put them together! I think the first step would be putting together a proposal of all the methods to include. Once we have the API ironed out in this thread, the implementation can happen. Do you have a proposal for how we should organize all the new auth API methods?

idanen commented 7 years ago

I haven't looked at the code yet, but I believe the $firebaseAuth service is indeed the right place and it sould have a wrapping class around firebase's User and implement all methods by wrapping them with a promise generate by $q like $q.resolve() or $q.when(). Makes sence?

jwngr commented 7 years ago

Maybe. The thing is that AngularFire currently has all user auth methods off of the $firebaseAuth service. However, the new 3.x.x Firebase SDKs have different methods off of firebase.auth.Auth and others off of `firebase.User. We should probably move in that direction as well with this library.

On a similar note, wrapping all 30 or so methods in a promise that works with the $digest loop seems like a lot of work for little benefit. I wonder if we should instead just document how users can do this themselves or provide a helper method that takes a Firebase promise and returns a new one that is properly wrapped so it works with the $digest loop. I haven't thought through all the details myself, but that would be a first step to do in order to update the lib.

idanen commented 7 years ago

So to take the new v3.x.x approach IMO the current $firebaseAuth service would be the firebase.User and we'll expose a new service to handle the firebase.auth.Auth service's methods.

About the wrapping thing, I don't think we have a choice. Writing an AngularJS module without handling change detection ($digest in this case) means ignoring the framework we're writing for. We could add a config (that should be on by default) opting whether to $digest but that would do the opposite of saving work that needs to be done :smile:

katowulf commented 7 years ago

Let's not wrap the SDK if we have an alternative

About the wrapping thing, I don't think we have a choice.

A recipe or example of how to accomplish auth effectively would be a decent compromise on cost vs. value and take us most of the way to an answer here, while avoiding the excessive wrappage.

A generic service that accepts an auth promise?

Perhaps another alternative would be to provide an auth service that accepts an auth handler and handles the digest portion, without having to wrap all the methods. Usage might look something like this:

app.service('authenticateUsingEmail', function($firebaseAuth) {
    return function(email, password) {
         // handleV3Login represents our service that helps with $digest et al
         // it receives a promise representing one of the firebase.auth methods
         // and (maybe?) it returns a promise that resolves to authUser
         $firebaseAuth.handleV3Login(
               firebase.auth().signInWithEmailAndPassword(email, password);
         );
    };
});

app.controller('LoginController', function(authenticateUsingEmail, $scope) {
    $scope.signIn = authenticateUsingEmail;
}

Maybe we don't need either?

One other thought occurs to me here. If we simply monitor firebase.auth().onAuthStateChanged() and trigger $digest any time it is triggered, then maybe we don't need either of the above. One could simply call firebase.auth().<auth method>() and $digest could still be triggered appropriately since onAuthStateChanged() will be triggered accordingly.

I think I like this answer the best.

idanen commented 7 years ago

I looked at it the wrong way. @katowulf, I totally agree with your last point. Since there's already that beautiful listener for auth changes, we can depend on that for change detection. The documentation should say implicitly that the framework assumes that this is the way consumers use. Maybe add some code samples.

The generic service seems redundant since consumer can use $q the exact same way.

So are you suggesting leaving the service as it is (keeping the User instance as an internal state that is exposed using $getAuth() or getCurrentUser()) and implement by simply calling the SDK's methods?

cbrunnkvist commented 7 years ago

Great, we just spent about one day running in circles while trying to figure out why getRedirectResult wasn't working / wasn't even there. Assumed it was an issue with require-js but as you understand that was a red herring.

Ok. We'll throw out the angularfire module and just load firebase directly, it works perfectly and is always up to date with itself.

jwngr commented 7 years ago

Note that even though AngularFire doesn't have these methods implemented just yet, you can always drop down to the vanilla JavaScript library while using AngularFire. You don't have to choose between them and can instead use both in conjunction.

marvinjude commented 6 years ago

The support for some new auth methods not found in the angularfire library Is saddening. I have a huge project in angular 1.x.x that need some of the new auth features provided by firebase 3.x.x . Are they ever gonna be implemented?

jamesdaniels commented 3 years ago

Closing as stale. At this point we consider this library complete and are advising remaining developers move to Angular and @angular/fire.