refactor: screen rewritten as functional component #2111
Conversation
alduzy
changed the title
There was a problem hiding this comment.
Hey, great job 🎉 I've got few remarks and questions we need to answer before we can merge these changes.
The rest of changes look very promising, but I would ask you to test them within our TestsExample application in following scenarios:
- using native-stack v5 (
createNativeStackNavigatorimported fromreact-native-screens - using native-stack v6 (
createNativeStackNavigatorimported from@react-navigation/native-stack
Please be aware of prop naming differences between v5 & v6.
We need to check few selected tests (1072, 1069 and some with reanimated should be amongst them) whether we don't have any undesired behaviour in downstream projects.
src/components/Screen.tsx
Outdated
| const closing = new Animated.Value(0); | ||
| const progress = new Animated.Value(0); | ||
| const goingForward = new Animated.Value(0); |
There was a problem hiding this comment.
These are moved from state to being recreated on every rerender. Is that intentional? Do we expect any issues here? @WoLewicki asking for some insights here. I'm not really familiar with semantics of Animated values.
There was a problem hiding this comment.
Maybe leveraging the useRef hook to store the values would be a good idea? What do you think?
There was a problem hiding this comment.
That was also my initial thought & I think we should go for this.
In any case I would like to hear from someone who might now some internals (already tagged).
There was a problem hiding this comment.
Yeah, I can see that in the example they use useRef for it: https://reactnative.dev/docs/animated#example. If it works correctly then it seems like the way to go. I can also see that a similar concept is used in react-navigation: https://github.com/react-navigation/react-navigation/blob/d0abdee67f5db8cf39112af535846ffededfb21d/packages/react-native-tab-view/src/useAnimatedValue.tsx#L4
| setNativeProps(props: ScreenProps): void { | ||
| this.ref?.setNativeProps(props); | ||
| } |
There was a problem hiding this comment.
Before we can remove this method as unused, we need to make sure no one actually uses it 😅 We need to:
- check
react-navigationcode for any usages of these, - as this seems like method that could be utilised by
react-native-reanimated- we need to also look for usages there (also see ourReanimatedScreenProvider) - it would be best to consult withreanimatedteam, - thinking now, I would rather leave this method exposed in this PR 😄
There was a problem hiding this comment.
Or is this intentional & you are utilising this method defined on innerRef?
There was a problem hiding this comment.
I believe Animated still uses setNativeProps under the hood so I'd leave it. After quick talk with @piaskowyk, react-native-reanimated is not utilizing it, but still we would want it for Animated. @alduzy did you maybe check examples with Animated to see if they work when this code is removed? Would be also good to check with both useNativeDriver set to false and true and in TestsExample and FabricTestsExample.
|
|
||
| setRef = (ref: React.ElementRef<typeof View> | null): void => { | ||
| this.ref = ref; | ||
| this.props.onComponentRef?.(ref); |
There was a problem hiding this comment.
I would like to see some comment why do we resign from calling this callback here (onComponentRef)
There was a problem hiding this comment.
That's an oversight, brought it back
| export const InnerScreen = React.forwardRef<View, ScreenProps>( | ||
| function InnerScreen(props, ref) { | ||
| const innerRef = React.useRef<ViewConfig | null>(null); | ||
| React.useImperativeHandle(ref, () => innerRef.current!, []); |
There was a problem hiding this comment.
This code is intended to replace removed code mentioned above:
setNativeProps(props: ScreenProps): void { this.ref?.setNativeProps(props); }
However I am not sure wether it is correctly implemented
Description
This PR intents to change Screen component into a functional component.