Skip to content
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

Fix Memory leak in BottomSheetBackdrop #1167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tsukudabuddha
Copy link

@tsukudabuddha tsukudabuddha commented Nov 4, 2022

Motivation

  • Fix Memory leak in BottomSheetBackdrop that occurs when the backdrop tries to set the pointerEvents to none when dismissing the modal. If the disappearsOnIndex is set to -1 and the animatedIndex is <= -1 , there is no need to call setEventPointers since the modal should be removed.

Fix Memory leak in BottomSheetBackdrop that occurs when the backdrop tries to set the `pointerEvents` to `none` when dismissing the modal even though we don't need to. If the `disappearsOnIndex` is set to `-1` and the `animatedIndex` is `<= -1` , there is no need to call `setEventPointers` since the modal should be removed.
@tsukudabuddha
Copy link
Author

here's the patch if anyone is interested:

diff --git a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx
index 35597ce..964bc25 100644
--- a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx
+++ b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx
@@ -105,7 +105,7 @@ const BottomSheetBackdropComponent = ({
   useAnimatedReaction(
     () => animatedIndex.value <= disappearsOnIndex,
     (shouldDisableTouchability, previous) => {
-      if (shouldDisableTouchability === previous) {
+      if (shouldDisableTouchability === previous || disappearsOnIndex === -1) {
         return;
       }
       runOnJS(handleContainerTouchability)(shouldDisableTouchability);

@rgomesrn
Copy link

using local patch until we wait for this to get merged, @gorhom appreciate when you have some time to review it

Copy link

@rgomesrn rgomesrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @tsukudabuddha

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@gorhom gorhom self-assigned this Jan 14, 2023
@gorhom gorhom added v4 Written in Reanimated v2 and removed no-pr-activity labels Jan 14, 2023
@vijaychouhan-rails
Copy link

@gorhom waiting for this PR to be merged.
Could you please check this PR and merge

@HyopeR
Copy link

HyopeR commented Feb 17, 2023

In my experience this fix prevents the bottom-sheet from opening on the first try. This pull request needs to be developed. Be careful before use.

@gorhom
Copy link
Owner

gorhom commented Apr 30, 2023

@tsukudabuddha thanks for submitting this PR, do you have a detailed issue reported i could verify the fix with?

@donatoaguirre24
Copy link

@gorhom here's an issue with more details: #1376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 Written in Reanimated v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants