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

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

Open
willbattel opened this issue Jul 9, 2020 · 4 comments
Labels

Comments

@willbattel
Copy link

Step 1: Are you in the right place?

Yes

Step 2: Describe your environment

  • Objective C or Swift: Swift
  • iOS version: >=11.1
  • Firebase SDK version: 6.27.1
  • FirebaseUI version: 8.4.2
  • CocoaPods Version: 1.9.2

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
Copy link
Author

willbattel commented Jul 9, 2020

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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.

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

No branches or pull requests

2 participants