-
Notifications
You must be signed in to change notification settings - Fork 282
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(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
ecce02d
Compare
!snapshot |
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/[email protected] --save-exact
npm i [email protected] --save-exact |
return; | ||
} | ||
|
||
if (numericTypes.includes(AuthenticationType.FINGERPRINT)) { |
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 the device supports multiple authentication methods (fingerprint and facial recognition), are we prioritizing fingerprint here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓Is the purpose of ignore
to not re-fetch isEnrolledAsync
?
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 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.
!snapshot |
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/[email protected] --save-exact |
biometryType: BiometricType | null; | ||
}; | ||
|
||
const LocalCredentialsInitValues: LocalCredentialsReturn = { |
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.
❓ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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.
packages/expo/src/hooks/useLocalCredentials/useLocalCredentials.ts
Outdated
Show resolved
Hide resolved
setUserOwnsCredentials(false); | ||
}; | ||
|
||
const authenticate = async () => { |
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.
💡 It would be nice to (optionally?) set the credentials after a successful auth
packages/expo/src/hooks/useLocalCredentials/useLocalCredentials.ts
Outdated
Show resolved
Hide resolved
packages/expo/src/hooks/useLocalCredentials/useLocalCredentials.ts
Outdated
Show resolved
Hide resolved
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.
💯
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 test
runs as expected.npm run build
runs as expected.Type of change