fix(auth): Fix auth type definitions and add more #8323
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
MichaelVerdon
changed the title
packages/auth/__tests__/auth.test.ts
Outdated
| @@ -1,4 +1,6 @@ | |||
| import { afterAll, beforeAll, describe, expect, it } from '@jest/globals'; | |||
| // @ts-ignore test | |||
| import FirebaseModule from '../../app/lib/internal/FirebaseModule'; | |||
packages/auth/lib/modular/index.d.ts
Outdated
| @@ -208,13 +224,13 @@ export function sendPasswordResetEmail( | |||
| * | |||
| * @param auth - The Auth instance. | |||
| * @param email - The user's email address. | |||
| * @param actionCodeSettings - Optional. Action code settings. | |||
| * @param actionCodeSettings - No longer optional from JS Docs. Action code settings. | |||
There was a problem hiding this comment.
interesting, it appears to be non-null across the board on this one:
https://firebase.google.com/docs/reference/js/auth.md?_gl=1*1c9e4k0*_up*MQ..*_ga*MTUyNTI2NTY2My4xNzM5MjkyNDkz*_ga_CW55HF8NVT*MTczOTI5MjQ5My4xLjAuMTczOTI5MjQ5My4wLjAuMA..#sendsigninlinktoemail_95b079b
The only thing I would say is; this might constitute a breaking change.
| @@ -309,13 +325,15 @@ export interface ApplicationVerifier { | |||
| * | |||
| * @param auth - The Auth instance. | |||
| * @param phoneNumber - The user's phone number. | |||
| * @param forceResend - Forces a new message to be sent if it was already recently sent. | |||
There was a problem hiding this comment.
Note this differs from firebase-js-sdk which doesn't have forceResend option:
This is specific to RNFB, example: https://github.com/invertase/react-native-firebase/blob/main/packages/auth/android/src/main/java/io/invertase/firebase/auth/ReactNativeFirebaseAuthModule.java#L1080
@mikehardy - should we keep it? Probably should I think but happy to stay true to firebase-js-sdk fidelity
There was a problem hiding this comment.
There are a few cases where native is a superset of firebase-js-sdk and I think, philosophically, that's incredible - it's the reason you would use native vs just firebase-js-sdk. So I strongly support implementing the native-only stuff when it appears.
In order to maintain drop-in API compatibility though, I think it should follow these general rules:
- any native-only parameters should be after any firebase-js-sdk parameters so that their optionality doesn't affect optional firebase-js-sdk params (this applies here! the order should be flipped I think)
- they should be marked clearly as for whatever platform they are exclusive to
- if they are a type and it means our new type shape isn't the same as the firebase-js-sdk shape, we should extend their type to add the new property/properties, with
ReactNativeprepended to the type name (e.g.,ReactNativeFirebaseAppshould extendFirebaseApp,ReactNativeFirebaseErrorshould extendFirebaseErroretc
In this case I think it's 1) keep the param but 2) move the param to be after all the optional firebase-js-sdk upstream params
| @@ -642,7 +672,7 @@ export function updateProfile( | |||
| export function verifyBeforeUpdateEmail( | |||
| user: FirebaseAuthTypes.User, | |||
| newEmail: string, | |||
| actionCodeSettings?: FirebaseAuthTypes.ActionCodeSettings, | |||
| actionCodeSettings?: FirebaseAuthTypes.ActionCodeSettings | null, | |||
There was a problem hiding this comment.
It does seem a touch redundant to have the type as actionCodeSettings? and also have | null. but firebase-js-sdk has it, so I guess it stays
| @@ -659,6 +689,273 @@ export function getAdditionalUserInfo( | |||
| * Returns the custom auth domain for the auth instance. | |||
| * | |||
| * @param auth - The Auth instance. | |||
| * @returns A promise that resolves with the custom auth domain. | |||
| * @returns {string} A promise that resolves with the custom auth domain. | |||
There was a problem hiding this comment.
this returns * @returns {Promise<string>}, one day it should be just string but that is when RNFB adopts new architecture with sync API calls to native.
| * Not in JS Docs | ||
| * | ||
| */ | ||
| export class AppleAuthProvider { |
There was a problem hiding this comment.
Are we sure the provider types that already exist on RNFB can't be imported out of lib/index.d.ts and reused?
| * | ||
| * | ||
| */ | ||
| export class OAuthProvider { |
There was a problem hiding this comment.
Following your comment at #8368 (comment), I had a look at this PR. It does indeed seem to add the exports I'm missing, but I'm a bit confused about what seems like a mismatch between the runtime and typed structure of the provider classes. For example, OAuthProvider seems to have very different methods at runtime compared to these types: https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/providers/OAuthProvider.js#L18
Or am I simply misunderstanding something?
There was a problem hiding this comment.
I understand what you are saying, what we try to do here is match the React Native implementation with the Javascript one but seeing those other methods are there in that class there must be a reason they are there. I will confirm it with the rest of the team.
There was a problem hiding this comment.
The older namespaced API has the addition of these seemingly extra methods.
There was a problem hiding this comment.
They seem to be present at runtime in the upstream Javascript SDK too through inheritance from BaseOAuthProvider and FederatedAuthProvider:
https://github.com/firebase/firebase-js-sdk/blob/dcfb3da2e65b25d12e1d53efb257440c9f026072/packages/auth/src/core/providers/facebook.ts#L66
https://github.com/firebase/firebase-js-sdk/blob/main/packages/auth/src/core/providers/oauth.ts#L63
Description
Added and edited type definitions to match Firebase JS-SDK with tests for definitions. Furthermore, this should fix this issue
Related issues
Release Summary
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter