search

firebase / FirebaseUI-iOS

iOS UI bindings for Firebase.
Apache License 2.0
1.51k stars 475 forks source link

FR: Add flag to FUIAuthDelegate didSignInWith to indicate if anonymous auto-upgrade occurred #883

Open willbattel opened 4 years ago

willbattel commented 4 years ago

Step 1: Are you in the right place?

Yes

Step 2: Describe your environment

Step 3: Describe the problem:

We're using FUI's shouldAutoUpgradeAnonymousUsers feature to, well, auto upgrade anonymous users. In the didSignInWith function for the FUIAuthDelegate we want to determine if the auto-upgrade was applied, or if the user already had a non-anonymous provider linked. In other words: we want to know if the "sign in" should be considered as a "sign up" where the user used a non-anonymous provider for the first time.

There are numerous work-arounds we can perform on our end to perform this same task, but built-in functionality would be a nice feature in the long run.

willbattel commented 4 years ago

I might try to work on a PR for this, if I find enough time to set aside. The only problem I see up front is that the function signature authUI(_:didSignInWith:error:) doesn't leave any room for this flag unless I either 1) add a new parameter to the function, or 2) tack the property onto the FUIAuth instance provided in the function. The former is obviously not optimal, and the latter isn't perfect either but I think it's preferable.

My tentative plan is to add a didUpgradeAnonymousUser read-only boolean property to the FUIAuth class that is set during every sign in. This property can be read within the didSignInWith function to check the state for the most-recent sign in.

Still brainstorming other solutions but from what I've come up with so far this is, in my opinion, the best option for minimal API impact.

morganchen12 commented 4 years ago

IMO the first solution is preferable, because the boolean didUpgradeAnonymousUser is scoped by the callback to the only place where it makes sense. Otherwise, you end up with a mutable bit on a global object that is only meaningful during a particular function call.

We can make this a non-breaking change by adding a new delegate method, deprecating the old one, and then having FUIAuth call both delegate methods if they're available.

willbattel commented 4 years ago

If I'm not mistaken, there are currently three didSignInWith delegate methods in play, one of which is deprecated. So is your idea to add a fourth and deprecate the other two...? Part of the problem is that one of the methods takes a URL and one does not, and now if we add another item you end up with a number of combinations each with its own function:

  1. FIRAuthDataResult*, NSError*
  2. FIRAuthDataResult*, NSURL*, NSError*
  3. FIRAuthDataResult*, BOOL, NSError*
  4. FIRAuthDataResult*, BOOL, NSURL*, NSError*

Maybe it would be best to deprecate both existing functions and add only function 4 with all parameters nullable? Then the developer can have all items available and only use what they need for their application.

morganchen12 commented 4 years ago

I'll bring this up with some of the other Firebase staff and brainstorm a design. I'm not opposed to having more callbacks, as long as it's clear when/where they should be used. In this case I think if people implement the one method with the signature they want they should be able to ignore the other three. But for clarity it is always nice to have fewer redundant methods.