-
Notifications
You must be signed in to change notification settings - Fork 485
feat: anonymous account upgrade with error handling #1247
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
base: main
Are you sure you want to change the base?
Conversation
FirebaseSwiftUI/FirebaseAuthSwiftUI/Sources/Services/AuthService.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the one comment.
@@ -167,21 +173,37 @@ public final class AuthService { | |||
} | |||
} | |||
|
|||
public func handleAutoUpgradeAnonymousUser(credentials credentials: AuthCredential) async throws { | |||
do { | |||
try await currentUser?.link(with: credentials) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle the currentUser
null state in a separate conditional, otherwise this do-catch has the possibility of executing nothing and returning no error. From a convenience perspective it may also be nice to pass the unwrapped user through to the AccoutMergeConflictContext
so the consumer doesn't have to again unwrap currentUser
.
In theory currentUser
should never be null here, so throwing an error is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@morganchen12 - updated. I didn't add User to the exception as it produced an Xcode compiler error: Stored property 'user' of 'Sendable'-conforming struct 'AccountMergeConflictContext' has non-sendable type 'User'
It seems User is non-sendable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fix for that is in the pipeline in upstream Firebase, so maybe leave a TODO and just send the user's UID (String) for now?
Notes
email-already-in-use
like in the current FirebaseUI Auth. This is logic is duplicated in a few place in previous implementation but is handled in one spot for FUIAuth SwiftUI.Questions
email-link
isn't in app storage, we throw an error here: https://github.com/firebase/FirebaseUI-iOS/pull/1247/files#diff-659d4c231d40710232200eb4482235de5964068a1395944bc32983fe0ed8670cR305. We could just push user back to email link sign-in View if it isn't present.