-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
WorkletEventHandler revamp #5845
Conversation
Please don't merge until #5790 is merged. |
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.
First part of the review is here - we definitely need to address components with multiple handlers.
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.
Looks good now, once we handle the blocker I think it'll be mergeleable.
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 wait for @tomekzaw's review (one day)
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 a couple of suggestions but all of them are optional
## Summary `registerEventHandler` and `unregisterEventHandler` methods in JS Reanimated were empty so far, but used in `WorkletEventHandler`. Since #5845 makes sure they are not used anymore, we can throw errors instead of keeping them empty. Note: to be merged after #5845 gets merged. ## Test plan :shipit:
return () => { | ||
eventHandler.workletEventHandler.unregisterFromEvents(); | ||
if (scrollRefTag.current !== null) { |
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.
Note generally reading a mutable reference in effect cleanup is risky because it could've changed by the time you got to cleanup (e.g. it could've been ... cleaned up). Or replaced by a newer reference. So it's a good idea to do
useEffect(() => {
const instance = someRef.current
doStuff(instance)
return () => {
undoStuff(instance)
})
})
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.
Yes, thanks for pointing out - luckily newer versions have already made it this way 😄
## Summary There have already been some requests for a way to merge our event handlers. Inspired by #5854, I created a hook that can work with any handlers made using `useEvent`. I will happily accept any comments and suggestions regarding typing since I am not so sure about it. https://github.com/software-mansion/react-native-reanimated/assets/77503811/9c295325-28ce-4ec2-8490-1a7431e8fafd The PR can be merged only ater #5845 (it is also based on it). ## Test plan Open `useComposedEventHandler` example from Example app and watch event callbacks in console.
## Summary Turns out (thanks to @j-piasecki for noticing) that animated component's event-emitting viewTag can change through re-rendering. The following fix takes care of that. ## Test plan You can go through all `ComposedEventHandler` examples form Example app, as well as code snippets in #5845 (comment). Also, this should work perfectly regardless of re-renders: <details><summary>Code</summary> ``` TYPESCRIPT import React, { useState } from 'react'; import { Button, StyleSheet, View } from 'react-native'; import { GestureDetector, Gesture } from 'react-native-gesture-handler'; import Animated, { useSharedValue, useAnimatedStyle, withSpring, } from 'react-native-reanimated'; function Ball(props) { const isPressed = useSharedValue(false); const offset = useSharedValue({ x: 0, y: 0 }); const animatedStyles = useAnimatedStyle(() => { return { transform: [ { translateX: offset.value.x }, { translateY: offset.value.y }, { scale: withSpring(isPressed.value ? 1.2 : 1) }, ], backgroundColor: isPressed.value ? 'yellow' : 'blue', }; }); const gesture = Gesture.Pan() .onBegin(() => { 'worklet'; isPressed.value = true; }) .onChange((e) => { 'worklet'; offset.value = { x: e.changeX + offset.value.x, y: e.changeY + offset.value.y, }; }) .onFinalize(() => { 'worklet'; isPressed.value = false; }); return ( <GestureDetector gesture={gesture}> <Animated.View style={[styles.ball, animatedStyles]} key={props.counter} /> </GestureDetector> ); } export default function Example() { const [counter, setCounter] = useState(0); return ( <View style={styles.container}> <Button title="Remount" onPress={() => setCounter((prev) => prev + 1)} /> <Ball counter={counter} /> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, }, ball: { width: 100, height: 100, borderRadius: 100, backgroundColor: 'blue', alignSelf: 'center', }, }); ``` </details>
Summary
Our
WorkletEventHandler
class (used inuseScrollViewOffset
anduseEvent
hooks as well in internal logic ofAnimatedComponent
) was made for handling only one component at a time. I made it so multiple of them can work at the same time. This way, anything based onuseEvent
(so for exampleuseAnimatedScrollHandler
) can be used for multiple components without needlessly copying the logic. This fixes #5345 and #5488 as well.Test plan
Effectively, this needs testing on
useAnimatedScrollHandler
anduseScrollViewOffset
as well as generally checking for any regression in AnimatedComponents.Testing code for
useAnimatedScrollHandler
on multiple components:Code
Testing code for
useScrollViewOffset
switching between components:Code
Testing code for multiple
useAnimatedScrollHandlers
and changing them on one component:Code
Regarding Animated Components, @Latropos currently works on some tests to make it easier.
Showcase of the last example:
scrollExample.mov