-
Notifications
You must be signed in to change notification settings - Fork 338
React liveness/provide default device info #6633
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
base: main
Are you sure you want to change the base?
React liveness/provide default device info #6633
Conversation
…ice automatically
🦋 Changeset detectedLatest commit: 3662944 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 |
Hello @riasatali42 , thanks for creating the PR! There's conflicts in the branch. Could you resolve the conflict and resubmit the PR? |
…aNotFoundCallback and device change callback methods after resolving merge conflicts
Hello @Simone319 , I have updated the branch with the current main branch and resolved all the conflicts. I've updated this PR instead of creating a new branch. Please let me know if I should do otherwise. |
@Simone319, I saw after running the tests that there were some ESLint issues. I've fixed those and updated the branch. |
data: { newDeviceId: selectedDeviceId, newStream }, | ||
}); | ||
} catch (error: any) { | ||
throw new Error('Error updating device:'); |
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.
Let's include the original error message. ex:
throw new Error(`Error updating device: ${error.message}`);
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.
Resolved.
onCameraChange(deviceInfo); | ||
} | ||
} catch (callbackError) { | ||
// console.error('Error in onCameraChange callback:', callbackError); |
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.
Here, we call still log the error, so that the error is not silently swallowed. However, we need to disable the eslint rule, such as:
// eslint-disable-next-line no-console
console.error('Error in onCameraChange callback:', callbackError);
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.
Resolved.
// console.error('Error in onCameraNotFound callback:', callbackError); | ||
// Don't rethrow to prevent state machine from getting stuck |
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.
Same as above.
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.
Resolved.
|
||
try { | ||
const deviceInfo = getSelectedDeviceInfo(context); | ||
onUserCancel(deviceInfo); |
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.
What's the use case of deviceInfo
in onUserCancel
callback?
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 main goal was to make all the callbacks more consistent. I've discarded it as we won't need this.
@@ -145,6 +145,40 @@ export const LivenessCameraModule = ( | |||
recording: 'flashFreshnessColors', | |||
}); | |||
|
|||
// Update the device if the selectedDeviceId changes | |||
React.useEffect(() => { |
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.
Why do we need to create a new effect here instead of reusing the existing onCameraChange callback?
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 we can discard that as the onCameraChange is already handling everything. So, I discarded.
const deviceInfo = getSelectedDeviceInfo(context); | ||
onUserCancel(deviceInfo); | ||
} catch (callbackError) { | ||
// console.error('Error in onUserCancel callback:', callbackError); |
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.
Same as above
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.
Resolved.
…rom the onUserCancel callback
Hello @Simone319 , I've updated the PR as per the feedback. |
Hi @riasatali42, thank you for addressing our concerns. Could you add end-to-end tests to verify the behavior of your changes? This guide should help you getting started: https://github.com/aws-amplify/amplify-ui/tree/main/packages/e2e If you got any questions, feel free to reach out. Thank you! |
Hello @adrianjoshua-strutt , Could you please guide me here to resolve this issue? |
Description of changes
This PR introduces features to pass in and out the device information (device Id, device label, kind, group Id) for the camera of the liveness check. Also, provides support to add callback methods for camera changes or default device not found.
Key additions:
Issue ##6587, Pass deviceId in and out of liveness check
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addeddocs
,e2e
,examples
, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.