-
-
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
Make Reanimated more compliant with rules of React #5942
base: main
Are you sure you want to change the base?
Conversation
|
||
export default function RuntimeTestsRunner() { | ||
const isRenderLocked = useRef<boolean>(false); |
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.
cc @Latropos
How to properly define this lock inside this component?
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.
Isn't it outdated already?
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.
maybe it is 😄
const outRef = ref ?? animatedRef; | ||
|
||
// @ts-expect-error TODO: Allow null as `useScrollViewOffset` ref argument. | ||
useScrollViewOffset(null, scrollViewOffset); |
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.
cc @szydlovsky
Pls make it work in useScrollViewOffset
(we are prisoners of our own API 💀)
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.
@tjzel do we really need nullable ref here that badly? Changing it will really complicate the hook implementation. The current native implementation also throws a warning when there is no ref - which I remember may have been your idea. After all, this hook with a null ref does essentially nothing so what do we need it for?
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.
Definitely we don't need to use nullable ref here. However:
- We shouldn't call this hook conditionally like before.
- The user can pass to
Animated.ScrollView
either:
2.1 An AnimatedRef - then we have no problems whatsoever.
2.2 A plain ref from React - then it will cause a runtime error.
In scenario 2.2 we would need to check if the passed ref is not an Animated one. If it's not, my idea would be to pass null
to useScrollViewOffset
hook as a special case that it shouldn't do anything. Keep in mind that we cannot just pass animatedRef
to it as for now, since in this scenario it's never getting attached to anything and causes another error.
I haven't came up with some other solution here that wouldn't be hacky. If you have a better solution though, I'm all ears.
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.
@tjzel get yourself a new main 👍
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.
🥳
@@ -41,13 +41,16 @@ export function useFrameCallback( | |||
}); | |||
|
|||
useEffect(() => { | |||
// Unregister previous callback. | |||
frameCallbackRegistry.unregisterFrameCallback(ref.current.callbackId); |
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.
Was unregistering here overlooked or is it not necessary? cc @tomekzaw @piaskowyk
## Summary #5942 needs `useScrollViewOffset`'s ref to be nullable - hence the PR. I also checked and a lot of cleanup code in the hook was redundant. ## Test plan You can run the following code, it checks both switching between nullable and non-nullable ref, as well as two different scrolls: (add some logs in hook's registering/unregistering to see that cleanups work fine) <details><summary>Code</summary> ``` TYPESCRIPT import React from 'react'; import Animated, { useAnimatedRef, useDerivedValue, useScrollViewOffset, } from 'react-native-reanimated'; import { Button, StyleSheet, Text, View } from 'react-native'; export default function EmptyExample() { const [refConnected, setRefConnected] = React.useState(false); const [refSwitched, setRefSwitched] = React.useState(false); const aref = useAnimatedRef<Animated.ScrollView>(); const bref = useAnimatedRef<Animated.ScrollView>(); const scrollHandler = useScrollViewOffset( refConnected ? (refSwitched ? bref : aref) : null ); useDerivedValue(() => { console.log(scrollHandler.value); }); const onButtonPress = () => { setRefConnected(!refConnected); }; const onSwitchButtonPress = () => { setRefSwitched(!refSwitched); }; return ( <> <View style={styles.positionView}> <Text>Test</Text> </View> <View style={styles.divider} /> <Button title={`${refConnected ? 'Disconnect' : 'Connect'} the hook`} onPress={onButtonPress} /> <Button title={`Change ref to the ${refSwitched ? 'first' : 'second'} scroll`} onPress={onSwitchButtonPress} /> <Animated.ScrollView ref={aref} style={[styles.scrollView, { backgroundColor: 'teal' }]}> {[...Array(100)].map((_, i) => ( <Text key={i} style={styles.text}> {i} </Text> ))} </Animated.ScrollView> <Animated.ScrollView ref={bref} style={[styles.scrollView, { backgroundColor: 'pink' }]}> {[...Array(100)].map((_, i) => ( <Text key={i} style={styles.text}> {i} </Text> ))} </Animated.ScrollView> </> ); } const styles = StyleSheet.create({ positionView: { margin: 20, alignItems: 'center', }, scrollView: { flex: 1, width: '100%', }, text: { fontSize: 50, textAlign: 'center', }, divider: { backgroundColor: 'black', height: 1, }, }); ``` </details>
@@ -91,7 +92,7 @@ const FlatListForwardRefRender = function <Item = any>( | |||
// @ts-expect-error In its current type state, createAnimatedComponent cannot create generic components. | |||
<AnimatedFlatList | |||
ref={ref} | |||
{...restProps} | |||
{...copiedProps} |
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.
Instead of copying, we could just also pass it here;
{...copiedProps} | |
{...restProps} | |
scrollEventThrottle={restProps.scrollEventThrottle ?? 1} |
no?
Summary
Test plan