-
Notifications
You must be signed in to change notification settings - Fork 305
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(A.Next): add useQRCodeString for RN #4886
Conversation
|
...t-native-auth/src/hooks/useQRCodeString/__tests__/__snapshots__/useQRCodeString.spec.ts.snap
Show resolved
Hide resolved
'^uuid$': '<rootDir>/../../node_modules/uuid', | ||
}, | ||
modulePaths: ['<rootDir>/node_modules/'], | ||
coverageThreshold: { |
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.
nice!
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.
LGTM other than some clarifying comments
}); | ||
|
||
return () => { | ||
ignore = true; |
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 this because the hook can be unmounted before the async toQRCodeString completes?
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.
Yep 😅
import { isFunction } from '@aws-amplify/ui'; | ||
|
||
export type UseQRCodeStringParams = { | ||
onError?: (err: string) => void; |
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.
Do we have a general pattern or philosophy on whether it should be onError?: (error: Error) => void;
or onError?: (err: string) => void;
?
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.
Good question 😄 We do not, for internal hooks personally have been leaning towards (errorMessage: string) => void
for ease of use in client code but don't feel particularly strongly either way
Description of changes
Add
useQRCodeString
and unit tests for Totp configuration in RNAuthenticator
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.