-
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): linkDomain
support for ActionCodeSettings
#8335
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/auth/lib/index.d.ts
Outdated
@@ -1360,7 +1366,7 @@ export namespace FirebaseAuthTypes { | |||
* @error auth/invalid-verification-id Thrown if the credential is a auth.PhoneAuthProvider.credential and the verification ID of the credential is not valid. | |||
* @param provider A created {@link auth.AuthProvider}. | |||
*/ | |||
reauthenticateWithProvider(provider: AuthProvider): Promise<UserCredential>; | |||
reauthenticateWithRedirect(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.
@mikehardy - when I was writing unit tests, the User was mismatched with the FirebaseAuthTypes.User
type (nice benefit of tests in TS). So I had a look and realised reauthenticateWithProvider
isn't a method on RNFB User, it is an underlying method on native: https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/User.js#L123
Hence the type update here.
However, these two methods don't exist on firebase-js-sdk: https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/User.js#L116-L125
They don't exist on iOS or android either. The method I replaced in this type declaration reauthenticateWithProvider()
does exist on iOS though: https://firebase.google.com/docs/reference/ios/firebaseauth/api/reference/Classes/FIRUser?#-reauthenticatewithprovider:uidelegate:completion:
For reference:
Not sure what to do about that, not for this PR in any event but something we should possibly address.
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 think reauthenticateWithPopup(auth, provider, resolver?): Promise<UserCredential>
is the closest map as that has the return type we want even if we will never use the resolver
https://firebase.google.com/docs/reference/js/auth.md#reauthenticatewithpopup_41c0b31
using reauthenticateWithRedirect
is an even bigger API mis-match as it has no return and implies they need to call some other method to get the result
focused on the modular API here at least, namespaced should be similar?
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.
the modular API is fine as it is, we have methods for reauthenticateWithRedirect()
& reauthenticateWithPopup()
with corresponding types:
https://github.com/invertase/react-native-firebase/blob/deprecation_warnings_dynamic_links/packages/auth/lib/modular/index.d.ts#L540-L554
The issue we have is actually there are two methods on namespaced User API which don't have a type on namespaced type declaration:
Instead, we have a type that isn't available on namespaced User (note: this is pointing to main branch): https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/index.d.ts#L1363
The fix is to create types for those two namespaced methods and remove the type declaration for a method that doesn't exist for the namespaced user. I should have just done this in the first place 🙏
Hopefully the above makes sense 😄
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.
Actually, I take it back. I think you're right. I think this method is a bit misleading and should probably throw an exception (something like "web API is unsupported at this moment"): https://github.com/invertase/react-native-firebase/blob/deprecation_warnings_dynamic_links/packages/auth/lib/index.js#L422
it should probably be reauthenticateWithPopup()
that we use I think.
linkDomain
support for ActionCodeSettings
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 think there may be a better API fit, but I looked in modular JS reference, just realized there is not actually a modular implementation here but it should be on auth and exported ? Unsure if it is
My comment still stands for the namespaced API though, as the Popup flavor has the return type we want there as well
https://firebase.google.com/docs/reference/js/v8/firebase.User#reauthenticatewithpopup
Other than that I think the implementation of the actual feature you were implementing looks pretty good?
@@ -2576,6 +2576,10 @@ private ActionCodeSettings buildActionCodeSettings(ReadableMap actionCodeSetting | |||
builder = builder.setDynamicLinkDomain(actionCodeSettings.getString("dynamicLinkDomain")); | |||
} | |||
|
|||
if(actionCodeSettings.hasKey("linkDomain")) { |
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.
yarn lint:android
will probably alter this one and return exit code 1 prompting for a fix?
I noticed on remote-config feature custom signals there is a false positive on android lint, because that one had formatting issues as well but CI was just fine with them...will look at that separately
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.
Fantastic
Description
Related issues
Release Summary
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter