-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fix the Sign In, Sign Up with email & password and Facebook #176
base: master
Are you sure you want to change the base?
Conversation
@shehand kindly review this pull request. I am looking forward for feedback. |
<string name="fb_login_protocol_scheme">fb1234</string> | ||
<string name="facebook_client_token">PLACE_YOUR_CLIENT_TOKEN_HERE</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.
If these are configurations that should be configured by the developer, readme file should contain step by step guide to config 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.
Added configuration in old readme file
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.
Please review the update reademe pr #169, so that i can also update this comfig in new readme file.
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.
Please address the issues in #169 first.
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.
Sure, i will address this first
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.
Added this configuration, please review #169
const { user } = await signInWithCredential(auth, credential); | ||
|
||
const response = await fetch( | ||
`https://graph.facebook.com/v2.8/me?fields=id,first_name,last_name,email,birthday&access_token=${data.accessToken}` |
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 can use Promise.allSettled() for this. Would be an performance improvement on my perspective.
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.
I got this on internet
"Uses of await or Promise.allSettled() depends on the specific use case and the performance characteristics of the promises being used. In general, if the promises can execute in parallel and the outcome of rejected promises is not important, Promise.allSettled() can be a good choice. However, if the promises need to execute sequentially or the outcome of rejected promises needs to be handled specifically, await may be a better choice."
Also in our case the response of promises are dependent on each other.
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.
@shehand Whats your suggestion on it.
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.
Could you please explain that how they are depending in each other? I can see two awaited calls which do not depend on each other at all.
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.
Yes you are right, these two promises are independent of each other but they are dependent on the above promises, which are not visible in this snippet.
So, I can either execute these two promises in parallel using Promise.allSettled() and explicitly check for their success and failure (since Promise.allSettled() will wait for all functions passed in the array to either resolve or reject without rejecting itself),
or I can use Promise.all(), which will reject as soon as one of the functions passed in the array rejects.
const response = await fetch( | ||
`https://graph.facebook.com/v2.8/me?fields=id,first_name,last_name,email,birthday&access_token=${data.accessToken}` | ||
); | ||
const userData = await response.json(); |
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.
So then we can remove await in here
// Use the user's access token to authenticate with Firebase Authentication | ||
const credential = FacebookAuthProvider.credential(data.accessToken); | ||
const { user } = await signInWithCredential(auth, credential); | ||
|
||
const response = await fetch( | ||
`https://graph.facebook.com/v2.8/me?fields=id,first_name,last_name,email,birthday&access_token=${data.accessToken}` | ||
); | ||
const userData = await response.json(); | ||
const photoURL = `https://graph.facebook.com/${userData.id}/picture?height=120`; |
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 comment as above
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.
Please see here, this is the full snippet. Also, as per your suggestion, I have already created a separate function for line 92. (This is an old snippet).
@shehand review the pr again, I have addressed most of the suggested changes and for some of the changes i have commented the reasons, please review them also. |
I have tested sign in and signup with email & password and facebook multiple time both of them working fine on client as well as on firebase. |
Also, this pull request includes the latest version configuration of Facebook SDK, which upgrades from react-native-fbsdk to react-native-fbsdk-next, as react-native-fbsdk package maintenance has been stopped since from last 2 years. |
@shehand review this pull request. |
const { user } = await signInWithCredential(auth, credential); | ||
|
||
const response = await fetch( | ||
`https://graph.facebook.com/v2.8/me?fields=id,first_name,last_name,email,birthday&access_token=${data.accessToken}` |
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.
Could you please explain that how they are depending in each other? I can see two awaited calls which do not depend on each other at all.
<string name="fb_login_protocol_scheme">fb1234</string> | ||
<string name="facebook_client_token">PLACE_YOUR_CLIENT_TOKEN_HERE</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.
Please address the issues in #169 first.
Fixed signup, signin #170
Summary:
This pull request involves fix and updates in the signup, signin feature of the Go-social app.
Issues:
Issue resolved Checklists:
Recording:
Project.1.mp4
These changes will ensure that the app is using the latest and most secure versions of the Firebase and Facebook SDKs, which will improve the overall security and stability of the application.
@shehand Please let me know if there are any issues or concerns with this pull request. I am happy to address any feedback or suggestions that you may have.