chore: move start screen skipping to ui and away from state machine #4799
Conversation
🦋 Changeset detectedLatest commit: b960bc5 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 |
packages/react-liveness/src/components/FaceLivenessDetector/FaceLivenessDetectorCore.tsx
Show resolved
Hide resolved
| @@ -38,6 +38,7 @@ interface LivenessCheckProps { | |||
| streamDisplayText: Required<StreamDisplayText>; | |||
| errorDisplayText: Required<ErrorDisplayText>; | |||
| components?: FaceLivenessDetectorComponents; | |||
| beginLivenessCheck: () => void; | |||
There was a problem hiding this comment.
Does this new prop affect customers, or is this only used internally?
There was a problem hiding this comment.
only used internally
packages/react-liveness/src/components/FaceLivenessDetector/FaceLivenessDetectorCore.tsx
Show resolved
Hide resolved
packages/react-liveness/src/components/FaceLivenessDetector/FaceLivenessDetectorCore.tsx
Show resolved
Hide resolved
| }); | ||
| }, [send]); | ||
|
|
||
| React.useLayoutEffect(() => { |
There was a problem hiding this comment.
Unless we have a compelling reason for useLayoutEffect here think useEffect should suffice. useLayoutEffect use cases are related to mutating the DOM/performing layout measurements and is documented to be potential performance bottleneck
| const [state, send] = useActor(service); | ||
| const isStartView = state.matches('start'); | ||
|
|
||
| const beginLivenessCheck = React.useCallback(() => { |
There was a problem hiding this comment.
As much as i am a supporter of deduping, think in this case it makes sense to move the body of beginLivenessCheck inside the useLayoutEffect where it is called and keep the existing beginLivenessCheck here where it currently is. Cuts down on memoization overhead in LivenessCheck , allows for this PR to be a simpler change, and prevents additional coupling between FaceLivenessDetectorCore/LivenessCheck
| @@ -43,6 +43,21 @@ export default function FaceLivenessDetectorCore( | |||
| }, | |||
| }); | |||
|
|
|||
| const [state, send] = useActor(service); | |||
| const isStartView = state.matches('start'); | |||
There was a problem hiding this comment.
disableStartScreen and isStartView read as somewhat contradicting one another imo, and since the existing declarations of isStartView are declared as:
const isStartView = state.matches('start') || state.matches('userCancel');
Maybe we can have a variable name spaced as something like shouldBeginLivenessCheck , example:
const shouldBeginLivenessCheck = disableStartScreen && state.matches('start');
Which would simplify the logic inside/deps required in the callback passed to useLayoutEffect
| const beginLivenessCheck = React.useCallback(() => { | ||
| send({ | ||
| type: 'BEGIN', | ||
| }); | ||
| }, [send]); |
There was a problem hiding this comment.
As noted in FaceLivenessDetectorCore, think the beginLivenessCheck declaration should remain here. Additionally, we don't need useCallback here, beginLivenessCheck is just being passed to a Button which is not an expensive re-render, and the Button is only rendered when isStartView is true
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addedsideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.