-
Notifications
You must be signed in to change notification settings - Fork 482
feat: init SwiftUI library for FirebaseAuthSwiftUI #1237
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
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.
Overall looks good. Had one architecture question for consideration.
FirebaseSwiftUI/FirebaseAuthSwiftUI/Sources/FirebaseAuthSwiftUI/FirebaseAuthSwiftUI.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.
Everything that's marked TODO can be done in a future PR.
FirebaseSwiftUI/FirebaseAuthSwiftUI/Sources/Services/AuthService.swift
Outdated
Show resolved
Hide resolved
FirebaseSwiftUI/FirebaseGoogleSwiftUI/Sources/Views/GoogleButtonView.swift
Outdated
Show resolved
Hide resolved
FirebaseSwiftUI/FirebaseAuthSwiftUI/Sources/Services/AuthConfiguration.swift
Outdated
Show resolved
Hide resolved
FirebaseSwiftUI/FirebaseGoogleSwiftUI/Sources/Services/GoogleProviderSwift.swift
Outdated
Show resolved
Hide resolved
FirebaseSwiftUI/FirebaseAuthSwiftUI/Sources/Services/AuthService.swift
Outdated
Show resolved
Hide resolved
let kEmailsDoNotMatchError = "EmailsDoNotMatchError" | ||
let kUnknownError = "UnknownError" | ||
|
||
public class StringUtils { |
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.
Let's see if using SwiftUI's string localization is a more canonical way to support this.
See https://developer.apple.com/documentation/xcode/localizing-and-varying-text-with-a-string-catalog
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.
migrated to use string catalog : 5c31f91
(#1237)
Use SwiftUI string localization: 76fa8f3
(#1237)
import Security | ||
|
||
class FacebookUtils { | ||
static func randomNonce(length: Int = 32) -> String { |
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.
This looks very similar to the nonce code we need to implement for Apple.
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.
moved to CommonUtils: 2923384
(#1237) for use with Apple package
|
||
public init() {} | ||
|
||
private var limitedLoginBinding: Binding<Bool> { |
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.
This computed property does a lot. It is trying to sync with ATTrackingManager.trackingAuthorizationStatus, and also drives showUserTrackingAlert and limitedLogin.
We should simplify this.
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.
Here is an alternative solution that feels cleaner:
@State private var showUserTrackingAlert = false
@State private var trackingAuthorizationStatus: ATTrackingManager.AuthorizationStatus = .notDetermined
var isLimitedLogin: Bool {
trackingAuthorizationStatus != .authorized
}
public init() {
_trackingAuthorizationStatus = State(initialValue: ATTrackingManager.trackingAuthorizationStatus)
}
func requestTrackingPermission() {
ATTrackingManager.requestTrackingAuthorization { status in
Task { @MainActor in
trackingAuthorizationStatus = status
if status != .authorized {
showUserTrackingAlert = true
}
}
}
}
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.
updated to reduce scope of binding: 0d5d8ba
(#1237)
Not sure if you agree but a binding was still needed to stop toggle from switching if limited login wasn't possible.
set: { newValue in | ||
let trackingStatus = ATTrackingManager.trackingAuthorizationStatus | ||
if newValue == true, trackingStatus != .authorized { | ||
self.showUserTrackingAlert = true |
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.
Instead of changing this from inside a computed property, I would recommend using the onChange(of:)
view modifier to listen to limitedLogin
and update showUserTrackingAlert
accordingly.
func deleteUser() async throws { | ||
do { | ||
if let user = auth.currentUser, let lastSignInDate = user.metadata.lastSignInDate { | ||
let needsReauth = !lastSignInDate.isWithinPast(minutes: 5) |
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.
Instead of making assumptions about the grace period, it'd be better to make the call and handle the error.
Work in progress