-
Notifications
You must be signed in to change notification settings - Fork 7
feat: add phone number authentication #110
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: development
Are you sure you want to change the base?
feat: add phone number authentication #110
Conversation
kefeh
left a comment
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 was a great start, I like the structuring and the implementations details, kudus. I left a few comments, hope it helps
...th_phone_number/authentication_with_phone_number_bloc/phone_number_authentication_state.dart
Outdated
Show resolved
Hide resolved
| Future<Either<AuthFailure, Unit>> verifyOTPCode({ | ||
| required String phoneNumber, | ||
| required Duration timeOut, | ||
| required PhoneVerificationFailed phoneVerificationFailed, |
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.
There are three problems i have with this.
- A function should have a maximum of 3 parameters and others can be compiled into a unit either map or class for proper readability etc.
- Avoid passing callbacks as parameters if you if you can.
- Most importantly, you are tightly coupling this facad with Firebase by depending on the firebase specific accessory functions and it defeats the purpose of a facad, you should be able to use even SuperBase and extend this facad without an issue
| required PhoneCodeAutoRetrievalTimeout autoRetrievalTimeout, | ||
| }) async { | ||
| try { | ||
| _firebaseAuth.verifyPhoneNumber( |
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.
What you can do here is define handlers for PhoneVerificationFailed, PhoneVerificationCompleted, PhoneCodeSent, PhoneCodeAutoRetrievalTimeout as accessory and private functions inside this Repository class and then call them here. If you realize, you see that verificationId is set and used only in this repository, so one thing you can do here is set is as a class variable initialized with an empty string, set the value in PhoneCodeSent and then use it inside verifyAndLogin
|
|
||
| abstract class IPhoneNumberRepositoryFacade { | ||
| Future<Either<AuthFailure, bool>> verifyAndLogin({ | ||
| required String verificationId, |
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.
same here, remove verificationId so we dont have coupling with firebase to the facad
fa00abb to
b050edb
Compare
b050edb to
07febba
Compare
- Configure native android files - Implement the logic with flutterbloc
07febba to
a45590f
Compare
Description
Implement phone number authentication using firebase and flutter bloc
Type of Change
Pre-launch Checklist
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy,///).