-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Change components implementation #3800
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
base: next
Are you sure you want to change the base?
Conversation
## What is happening While working on #3800 I've noticed frequent crashes on `iOS`. I'll explain this using the demo from the test code, basically two screens, second screen has a `RectButton`. 1. Navigate to second screen 2. Go back 3. Navigate to the second screen again 4. App crashes Sometime it crashes immediately on startup. ## Why is it happening? `Native` handlers are handled differently compared to others. The main problem was that we tried to attach handler that was previously dropped, but `NativeDetector` has no information that this happened. The flow here looks as follows: 1. We navigate to screen with button 2. `addSubview` is called - at this point it won't attach anything as `_nativeHandlers` is empty 3. `updateProps` is called - handlers are attached 4. We navigate back to first screen 5. Handler is dropped 6. We navigate again to second screen 7. `addSubview` is called - it tries to attach handler that was dropped, therefore app crashes ## Solution To fix this issue, we clear `_nativeHandlers` in `prepareForRecycle` method. ## Test plan <details> <summary>I've tested it on my branch with components re-written to new API, using the following code in EmptyExample:</summary> ```tsx import React from 'react'; import { StyleSheet, View } from 'react-native'; import { RectButton } from 'react-native-gesture-handler'; export default function EmptyExample() { return ( <View style={styles.container}> <RectButton style={styles.button} onPress={() => console.log('Hello World!')} /> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, justifyContent: 'center', alignItems: 'center', }, button: { width: 100, height: 30, borderRadius: 10, backgroundColor: 'pink', }, }); ``` </details>
...ve-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt
Outdated
Show resolved
Hide resolved
| val id = if (child is ReactSwipeRefreshLayout) { | ||
| child.getChildAt(0).id | ||
| } else { | ||
| child.id | ||
| } |
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.
- Why do we need that? It looks like it makes attaching to
ReactSwipeRefreshLayoutimpossible - Can this be done in the
NativeViewGestureHandlerHook?
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 looks like it makes attaching to
ReactSwipeRefreshLayoutimpossible
This should be handled by attachVirtualChildren, right?
Can this be done in the
NativeViewGestureHandlerHook?
We are inside NativeDetector, so I don't exactly know what do you mean. Something like handler.shouldAttachToChild that would return hook.shouldAttachToChild?
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.
If there's no way to get rid of it, can we get a comment explaining the "why" behind this workaround?
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.
For now I can't see a clean way of doing it in hook. Added a comment in 7ad7ad7.
| this.isInitialized = false; | ||
| } | ||
|
|
||
| configure(): void { |
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 don't really like the name. Maybe updateListeners?
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 don't like it either, but I don't think updateListeners fits here, as 2 functions inside are responsible for style, not for listening to events 😞
| // Backward type compatibility with https://github.com/software-mansion/react-native-gesture-handler/blob/db78d3ca7d48e8ba57482d3fe9b0a15aa79d9932/react-native-gesture-handler.d.ts#L440-L457 | ||
| // include methods of wrapped components by creating an intersection type with the RN component instead of duplicating them. | ||
| // eslint-disable-next-line @typescript-eslint/no-redeclare | ||
| export type ScrollView = typeof GHScrollView & RNScrollView; |
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 trickery still needed (and also for the rest of the components)?
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, but in slightly different form - 5f7ec9c.
packages/react-native-gesture-handler/src/v3/components/GestureComponents.tsx
Outdated
Show resolved
Hide resolved
| export const RefreshControl = createNativeWrapper( | ||
| RNRefreshControl, | ||
| { | ||
| disallowInterruption: true, | ||
| shouldCancelWhenOutside: false, | ||
| }, | ||
| DetectorType.Virtual | ||
| ); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-redeclare | ||
| export type RefreshControl = typeof RefreshControl & RNRefreshControl; | ||
|
|
||
| const GHScrollView = createNativeWrapper<PropsWithChildren<RNScrollViewProps>>( | ||
| RNScrollView, | ||
| { | ||
| disallowInterruption: true, | ||
| shouldCancelWhenOutside: false, | ||
| }, | ||
| DetectorType.Intercepting | ||
| ); |
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.
How does this work when, correct me if I'm wrong, Android and iOS render them in different positions, e.g.:
<RefreshControl>
<ScrollView />
</RefreshControl>
on one and
<ScrollView>
<RefreshControl />
</ScrollView>
on the other. I'd imagine that would also impact where the detector is rendered.
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've also thought about it for a while, but it doesn't. In case of ScrollView we have the following structure, thanks to createNativeWrapper:
<InterceptingGestureDetector>
<RNScrollView>Now, it doesn't matter which one will be rendered first RefreshControl or ScrollView - they both fall under this Detector. While React Native does change this order, it does it inside ScrollView, so effectively we have:
{/* Android */}
<InterceptingGestureDetector>
<RefreshControl>
<ScrollView>
{/* iOS */}
<InterceptingGestureDetector>
<ScrollView>
<RefreshControl>InterceptingGestureDetector stays at the same level.
…eComponents.tsx Co-authored-by: Jakub Piasecki <[email protected]>
| val view = if (GestureHandler.usesNativeOrVirtualDetector(handler.actionType)) { | ||
| handler.viewForEvents | ||
| handler.hostDetectorView!! | ||
| } else { | ||
| handler.view!! | ||
| } |
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.
What do you think about making this the new .viewForEvents?
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 like the idea! aabc5b4.
| ); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-redeclare | ||
| export type RefreshControl = RNRefreshControlProps; |
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.
Why is this one mapped to the Props type?
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.
My bad, I've missed that. Changed in 06428d4.
| val id = if (child is ReactSwipeRefreshLayout) { | ||
| child.getChildAt(0).id | ||
| } else { | ||
| child.id | ||
| } |
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.
If there's no way to get rid of it, can we get a comment explaining the "why" behind this workaround?
| /** | ||
| * @deprecated use `ScrollView` instead | ||
| */ | ||
| export const LegacyScrollView = React.forwardRef< |
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.
Why do we keep legacy implementations?
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.
We'd like to have a way for people to upgrade to 3.0 and keep their apps working as they did before. Since the new components aren't compatible API-wise with the old ones, it makes sense to keep both for the transition period.
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'd add that we do this because in that case it is easier - we want to leave old createNativeWrapper either way and those components don't have to be re-written, unlike those based on Reanimated.
I believe that's the root of this question - why Reanimated components don't have legacy versions.
| useImperativeHandle(props.ref, () => ({ | ||
| componentRef: componentRef.current, | ||
| gestureRef: gestureRef.current, | ||
| })); |
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 this is at all useful:
- It's a breaking change compared to the previous api and the components stop being a drop-in replacement -
scrollRef.current.scrollTobecomesscrollRef.current.componentRef.scrollTo. - Is
gestureRefactually usable? There's no way to react to changes happening to it in an easy way. It will benullafter creating the ref, and that's what will be used for relations during the first render. To actually read the ref, something would need to cause a render after it's set. If it happens to change (like in the case of child remount), there's no signal that the parent (gesture) should react to it.
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.
Ad 1. I agree that this may be a problem.
Ad 2. Isn't it necessary if we want to set relation between ScrollView and RefreshControl? For now RefreshControl blocks ScrollView, so we need that ref. On the other hand, we can't pass RefreshControl to ScrollView to wait for. I mean we could, but then we would have to call createNativeWrapper inside ScrollView component.
Am I missing something?
Description
This PR changes components implementations to use new hooks API.
Warning
It requires changes in module on iOS - call
createGestureHandlerimmediately, not adding operation block.Test plan
Example apps