Uh oh!
There was an error while loading. Please reload this page.
feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials - #3663
Conversation
🦋 Changeset detectedLatest commit: 6b6308d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
44fb76b to
ecce02dComparepanteliselef
commented
Jul 5, 2024
!snapshot |
clerk-cookie
commented
Jul 5, 2024
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/clerk-expo@1.3.0-snapshot.ve946474 --save-exact
npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact |
| return; | ||
| } | ||
| if (numericTypes.includes(AuthenticationType.FINGERPRINT)) { |
There was a problem hiding this comment.
❓If the device supports multiple authentication methods (fingerprint and facial recognition), are we prioritizing fingerprint here?
There was a problem hiding this comment.
That's a very nice question, tbh my intention here was to have AuthenticationType.IRIS behave like face-recognition so i might need to reverse the logic, in order for iris/face have priority
| export function LocalCredentialsProvider(props: PropsWithChildren): JSX.Element { | ||
| const { isLoaded, signIn } = useSignIn(); | ||
| const { publishableKey } = useClerk(); | ||
| const key = `__clerk_local_auth_${publishableKey}_identifier`; |
There was a problem hiding this comment.
💡Nit: Could we extract this interpolation to a separate function? Or perhaps type it, to avoid accidentally changing it or misspelling between individual variables.
| const [isEnrolled, setIsEnrolled] = useState(false); | ||
| useEffect(() => { | ||
| let ignore = false; |
There was a problem hiding this comment.
❓Is the purpose of ignore to not re-fetch isEnrolledAsync?
There was a problem hiding this comment.
So the intention is to avoid race conditions in a situation where the component gets unmounted but the promise resolves after the component is already unmounted.
Uh oh!
There was an error while loading. Please reload this page.
panteliselef
commented
Aug 6, 2024
!snapshot |
clerk-cookie
commented
Aug 6, 2024
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/clerk-expo@2.1.0-snapshot.vff0337d --save-exact |
| biometryType: BiometricType | null; | ||
| }; | ||
| const LocalCredentialsInitValues: LocalCredentialsReturn = { |
There was a problem hiding this comment.
❓ Should we have something like isPlatformSupported in LocalCredentialsInitValues in order to be easier to check if the hook can be used on specific platforms like Web or a device that does not has any biometric support, as now it seems you will have to check if biometryType is null which is not so clear if you don't take a look at the code
There was a problem hiding this comment.
For devices that do not support biometric, android and iOS fallback to PIN, and if a pin does not exist then biometricType will be null.
with biometricType you get the available authenticator and it is useful mostly for UI purposes. Displaying a Face ID or a touch ID icon for example.
Maybe simply changing the name to supportedBiometricType would do ?
We don't wanna indicate that the device can use biometric, but instead if we can use local authentication (biometric or not) in order to store credentials.
There was a problem hiding this comment.
I was mostly referring for platforms that this will not work at all like web,the biometric support was just an example as I didn't know if this fallbacks to pin
brkalow
left a comment
There was a problem hiding this comment.
Need some JSDoc comments on exported methods, and left a few non-blocking comments. Nice work!
Feel free to dismiss if you address the comments during your day tomorrow.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| setUserOwnsCredentials(false); | ||
| }; | ||
| const authenticate = async () => { |
There was a problem hiding this comment.
💡 It would be nice to (optionally?) set the credentials after a successful auth
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…s.ts Co-authored-by: Bryce Kalow <bryce@clerk.dev>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…s.ts Co-authored-by: Bryce Kalow <bryce@clerk.dev>
Description
This PR introduces a new hook API
Video of the new UX flow
RPReplay_Final1719504401.MP4
1.Example usage in a profile page
1.Example usage in a custom sign in flow
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change