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.

Sign up for GitHub

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

Task/103/android phone auth change #369

Closed
wants to merge 4 commits into from
Closed

Task/103/android phone auth change #369

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e32362
added callback to let android know the verifationId in case the autom…
Apr 8, 2023
83e8481
fixed method call name mishap
Apr 8, 2023
1d5d740
fixed unresolved variable
Apr 13, 2023
74a77c9
Merge branch 'master' into task/103/AndroidPhoneAuthChange
AKnght Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions firebase-auth/src/androidMain/kotlin/dev/gitlive/firebase/auth/credentials.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ actual class PhoneAuthProvider(val android: com.google.firebase.auth.PhoneAuthPr
PhoneAuthProvider.OnVerificationStateChangedCallbacks() {

override fun onCodeSent(verificationId: String, forceResending: PhoneAuthProvider.ForceResendingToken) {
verificationProvider.onRecievedVerificationID(verificationId = verificationId)
verificationProvider.codeSent { android.verifyPhoneNumber(phoneNumber, verificationProvider.timeout, verificationProvider.unit, verificationProvider.activity, this, forceResending) }
}

Expand Down Expand Up @@ -123,6 +124,7 @@ actual interface PhoneVerificationProvider {
val activity: Activity
val timeout: Long
val unit: TimeUnit
fun onRecievedVerificationID(verificationID: String)
fun codeSent(triggerResend: (Unit) -> Unit)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why separate method, instead of the codeSent's parameter?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to change a method signature. Also, less convoluted code is easier to read and maintain. Separation of concern. Separation of responsibility.

Copy link

@valeriyo valeriyo Apr 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it (changing method signature) matter? It doesn't seem like anyone is using gitlive's "sign in with phone" stuff.... because as soon as you try to use it - you run into this very problem (#104), which is a UX show-stopper.

For the separation of concerns, I think this is artificial. The Firebase documentation (https://firebase.google.com/docs/auth/android/phone-auth#oncodesentstring-verificationid,-phoneauthprovider.forceresendingtoken) is describing the verification process, where verificationId becomes known at the time of codeSent callback, and you'd be better off sticking to same semantics, to reduce the need to search for how gitlive does it.

Also, why is there Unit parameter in triggerResend: (Unit) -> Unit) -- should be just triggerResend: () -> Unit) - a normal signature of a method that doesn't take any parameters 🤷 You may as well add verificationId parameter, and fix triggerResend in one shot.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the last part but I have worked with many sdks and the smaller the pieces the better. Less siginture changes the better.

Either way this update is needed and had been needed for a very long time. If it doesn't happen soon I will have to shift into building and maintaining my own SDK for this which is what I am trying to avoid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api should follow android sdk apart from the kotlin-first design principles defined in the readme, is this the case for this?
If you paste in links to the relevant firebase sdk documentation I would be happy to confirm it for you

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

override fun onCodeSent(
verificationId: String,
token: PhoneAuthProvider.ForceResendingToken
) {

/~https://github.com/firebase/snippets-android/blob/076236ee54e3b6c1bab0152c52fdb3ebc5c0640a/auth/app/src/main/java/com/google/firebase/quickstart/auth/kotlin/PhoneAuthActivity.kt#L39-L81

Line 68

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android applications need that ID or they cant complete the phone code auth.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone else may need this so I'll leave it open but I am pivoting away from this.

suspend fun getVerificationCode(): String
}
Expand Down