-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(auth, oauth): add native oauth support / no external packages needed #7019
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
working for android, ios coming soon |
@cedricboidin has added ios |
Rebased on main. I would appreciate some feedback on this PR :) |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
Sincere apologies for the delay! I'm processing the PR queue now and should have time to review this, I sincerely appreciate the effort and the patience |
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 great! Happy to hear it's working for you in production that gives me a lot of confidence as I look to merge this.
I left a couple specific comments that are hopefully easy to handle
I rebased to current main and ran the linters so those hoops have already been jumped through
The only thing I can think of besides the specific comments is to add something in docs/auth
- might make sense to great make a page there (slicing in to the prev/next links on two of the related pages to get the circular nav links going) ?
Functionally CI should go green now though 🤞 with fully up to date SDKs etc etc
signInWithPopup(provider: AuthProvider): Promise<UserCredential>; | ||
signInWithRedirect(provider: AuthProvider): Promise<UserCredential>; |
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.
Can you document this here, even if briefly? The rnfirebase.io site is built from the typedoc so anything you put here will be seen + used
@@ -1950,6 +2074,7 @@ private WritableMap getJSError(Exception exception) { | |||
String message = exception.getMessage(); | |||
String invalidEmail = "The email address is badly formatted."; | |||
|
|||
System.out.print(exception); |
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.
Dumping an error to System.out is discouraged, if it is already being propagated up, then I think this could be removed?
Looks like another of the test cases needs a modification similar to those already made:
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
Summer vacation time was busy enough the stale bot got this one, but it's a good looking chunk of work, hoping to get it shepherded through for merge |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
scanning stale closed queue for unrelated reasons this still looks harvest-able |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
Description
Right now, documentation recommendation for oauth flows is to use the react-native-app-auth package.
What this package do is pretty close to the default behaviour of the firebase native sdk.
With the support of my company and of my teammates from kraaft.co, I had some time allocated to check whether I can bridge the native sdk with react native within react-native-firebase :)
The goals of such integration would be:
Related issues
#349
#3926
Release Summary
Checklist
Android
iOS
-> If anyone knows his swift I'm down for some help :)e2e
tests added or updated inpackages/\*\*/e2e
-> Would like to have some insights with detox and testing the popupjest
tests added or updated inpackages/\*\*/__tests__
Test Plan
we are deploying this on our staging, and will QA this soon
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter🔥