-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Add runOnJS
to config
#3743
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
Add runOnJS
to config
#3743
Conversation
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, left just 2 small questions
); | ||
} | ||
|
||
// This has to be done ASAP as other hooks depend `shouldUseReanimated`. |
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 comment still valid? After you removed the shouldUseReanimated
prop?
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 it's still needed since shouldUseReanimated
became shouldUseReanimatedDetector
, but it definitely needs to be updated.
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 is valid, changed to shouldUseReanimatedDetector
in 7a4a014
config.needsPointerData = shouldHandleTouchEvents(config); | ||
config.dispatchesAnimatedEvents = isAnimatedEvent(config.onUpdate); | ||
config.dispatchesReanimatedEvents = | ||
config.shouldUseReanimatedDetector && runOnJS !== 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.
I wonder if it can be extracted to a helper function. The similar logic is already in the packages/react-native-gesture-handler/src/v3/hooks/utils/reanimatedUtils.ts
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'm not sure if that makes sense - one value comes from config, other from shared value listener. Extracting it into a separate function would make it more complicated (like using maybeUnpackValue
inside), so I don't think it makes much sense.
Though I've unified checks in b470764 😅
...ve-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt
Show resolved
Hide resolved
); | ||
} | ||
|
||
// This has to be done ASAP as other hooks depend `shouldUseReanimated`. |
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 it's still needed since shouldUseReanimated
became shouldUseReanimatedDetector
, but it definitely needs to be updated.
Description
This PR adds
runOnJS
to gesture config.Test plan
Tested on the following example: