fix: Added dependency array for react native web project (crashing react native reanimated 3.X) #11496

agusvazquez · 2023-07-25T19:08:14Z

Please provide enough information so that others can review your pull request.

Motivation

Explain the motivation for making this change. What existing problem does the pull request solve?
I need to use react native reanimated v3 and I am facing the following issue on react native web.

#11483

If this pull request addresses an existing issue, link to the issue. If an issue is not present, describe the issue here.

#11483

Test plan
Describe the steps to test this change so that a reviewer can verify it. Provide screenshots or videos if the change affects UI.
Make sure the drawer works on react native web using react native reanimated v3.4.0

The change must pass lint, typescript and tests.

Signed-off-by: Agustin Vazquez <agustin.vazquez@copilotiq.com>

github-actions · 2023-07-25T19:08:26Z

Hey @agusvazquez! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

netlify · 2023-07-25T19:09:59Z

✅ Deploy Preview for react-navigation-example ready!

Name	Link
🔨 Latest commit	`46291c5`
🔍 Latest deploy log	https://app.netlify.com/sites/react-navigation-example/deploys/650b10fe0f84df00071324b2
😎 Deploy Preview	https://deploy-preview-11496--react-navigation-example.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Agustin Vazquez <agustin.vazquez@copilotiq.com>

agusvazquez · 2023-07-27T15:04:01Z

Hello! Can someone review this? Need this merged in so drawer works on react navigation web using RN Reanimated v3 and react native v 0.72.X.

Thank you!

radelcom · 2023-07-31T05:57:23Z

Hello! Can someone review this? Need this merged in so drawer works on react navigation web using RN Reanimated v3 and react native v 0.72.X.

Thank you!

@agusvazquez

just curious... i tested with a pactch-package with your changes but still not getting drawer to work properly in web. does it work only for react-natve v0.72.x? im currenly at v0.71.x

agusvazquez · 2023-07-31T13:31:35Z

Hello @radelcom . Thanks for taking a look.

To be 100% honest, I'm not sure. I just added the dependency array that react-native-reanimated v3.X.X needs in order to not crash on web. I tested those changes locally on my project and they worked fine.

Also just to make sure, I am using React Native web for this project.

I am using the following versions.

"react-native": "0.72.3",
"react-native-reanimated": "^3.4.1",
"react-native-web": "^0.18.12",

Sorry, this is my first time sending a PR to a big library like this, not sure how to do a proper test btw (my test consisted in manually modifying the node_modules folder with my changes 😆 ). I'm noob at this. Any suggestions are welcome.

codecov-commenter · 2023-08-01T09:10:09Z

Codecov Report

Patch coverage has no change and project coverage change: +0.17% 🎉

Comparison is base (c43208f) 75.49% compared to head (46291c5) 75.67%.
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11496      +/-   ##
==========================================
+ Coverage   75.49%   75.67%   +0.17%     
==========================================
  Files         190      202      +12     
  Lines        5742     5854     +112     
  Branches     2261     2304      +43     
==========================================
+ Hits         4335     4430      +95     
- Misses       1360     1372      +12     
- Partials       47       52       +5

see 47 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tjzel · 2023-08-01T09:15:24Z

It's possible to run react-native-reanimated on Web without Reanimated Babel plugin under one condition - you specify dependencies explicitly in useAnimatedReaction, useAnimatedStyle, useDerivedValue.
Therefore, this PR looks all good since it adds those fallbacks. Just double check if you added dependencies to all of those hooks present in the code.

agusvazquez · 2023-08-01T13:50:12Z

@tjzel Hello! I have just double checked and there were some hooks I was misisng. I have just updated the code. Please check!

Also I looked for useAnimatedReaction but I didn't find any occurrence in the project.

Hope this works! :)

Cheers

tjzel · 2023-08-01T14:12:45Z

Good that you noticed useAnimatedProps, I forgot about that one! 😅

tjzel · 2023-08-01T15:14:38Z

packages/react-native-drawer-layout/src/views/modern/Drawer.tsx

@@ -310,7 +310,7 @@ export function Drawer({
        : minmax(translationX.value - touchDistance, 0, drawerWidth);

    return translateX;
-  });
+  }, [touchStartX, drawerWidth, gestureState, drawerPosition, translationX]);


With react-native-reanimated/plugin auto-generated dependencies would be:
[drawerType, gestureState, GestureState, minmax, drawerPosition, touchStartX, drawerWidth, layout, translationX]. Unless there's good reason not to include some of them here I'd rather keep them all.

Thanks! I am updating this, however GestureState is an enum, I think we shouldn't put enums as dependencies.

Why shouldn't we? Had it been transpiled it would've been captured into dependencies.

Because it is a constant and it will never change.

Also, I am receiving the typescript error displayed above.

Well, in the future it could be mutable (although I strongly doubt it) and enums are transpiled into JS objects, but I guess it's fair enough to not include it here.

tjzel · 2023-08-01T15:18:17Z

packages/react-native-drawer-layout/src/views/modern/Drawer.tsx

@@ -337,7 +337,7 @@ export function Drawer({
              },
            ],
    };
-  });
+  }, [touchStartX, drawerWidth, gestureState, drawerPosition, translationX]);


Here it's:
[layout, drawerWidth, drawerType, translateX, drawerPosition, isRTL].

tjzel · 2023-08-01T15:20:09Z

packages/react-native-drawer-layout/src/views/modern/Drawer.tsx

@@ -357,7 +357,7 @@ export function Drawer({
              },
            ],
    };
-  });
+  }, [touchStartX, drawerWidth, gestureState, drawerPosition, translationX]);


Here:
[drawerType, translateX, drawerWidth, drawerPosition]

tjzel · 2023-08-01T15:20:55Z

packages/react-native-drawer-layout/src/views/modern/Drawer.tsx

@@ -367,7 +367,7 @@ export function Drawer({
          [getDrawerTranslationX(false), getDrawerTranslationX(true)],
          [0, 1]
        );
-  });
+  }, [translateX, drawerType]);


Here:
[drawerType, interpolate, translateX, getDrawerTranslationX]

tjzel · 2023-08-01T15:21:36Z

packages/react-native-drawer-layout/src/views/modern/Overlay.tsx

@@ -30,7 +30,7 @@ export const Overlay = React.forwardRef(function Overlay(
      // We can send the overlay behind the screen to avoid it
      zIndex: progress.value > PROGRESS_EPSILON ? 0 : -1,
    };
-  });
+  }, [progress, PROGRESS_EPSILON]);


tjzel · 2023-08-01T15:22:25Z

packages/react-native-drawer-layout/src/views/modern/Overlay.tsx

@@ -40,7 +40,7 @@ export const Overlay = React.forwardRef(function Overlay(
      accessibilityElementsHidden: !active,
      importantForAccessibility: active ? 'auto' : 'no-hide-descendants',
    } as const;
-  });
+  }, [progress, PROGRESS_EPSILON]);


tjzel · 2023-08-01T15:24:16Z

I would wait with merging until we understand what causes #11483

agusvazquez · 2023-08-04T20:10:49Z

Have pushed a new commit, please recheck @tjzel .

Thanks for the feedback!

sacru2red · 2023-09-04T00:11:46Z

ping. can we merge it??
@satya164 @osdnk

satya164

Thank you for the PR. Can you add the functions to the ESLint config with "react-hooks/exhaustive-deps"'s additionalHooks config so that any missing dependencies are caught by ESLint?

We mostly rely on the ESLint plugin to automatically add dependencies so it's crucial to configure ESLint to catch this scenario to avoid future bugs.

agusvazquez · 2023-09-20T15:35:22Z

Added it @satya164 . Let me know if this is ok.

Thank you!

satya164 · 2023-09-20T16:13:55Z

Thanks! Just few more things:

Change warn for the ESLint config to error since we need CI to fail if missing dependencies
Currently the lint stage on the CI is failing due to TypeScript issues. Make sure CI is passing to be able to merge the PR
There are lint warnings about unnecessary dependencies (minmax, interpolate, PROGRESS_EPSILON), remove them to fix the warnings

github-actions · 2023-10-26T01:43:30Z

Hello 👋, this pull request has been open for more than a month with no activity on it. If you think this is still necessary with the latest version, please comment and ping a maintainer to get this reviewed, otherwise it will be closed automatically in 7 days.

Updated drawer

612c81c

Signed-off-by: Agustin Vazquez <agustin.vazquez@copilotiq.com>

github-actions bot added the first pull request label Jul 25, 2023

agusvazquez mentioned this pull request Jul 25, 2023

Pass dependency array when calling useAnimatedStyle #11483

Open

11 tasks

agusvazquez changed the title ~~Added dependency array for web.~~ For Drawer: Added dependency array for react native web project. Jul 25, 2023

agusvazquez added 2 commits July 25, 2023 16:38

no message

6cc2410

Signed-off-by: Agustin Vazquez <agustin.vazquez@copilotiq.com>

Adding missing params to Overlay

9896dab

Signed-off-by: Agustin Vazquez <agustin.vazquez@copilotiq.com>

agusvazquez changed the title ~~For Drawer: Added dependency array for react native web project.~~ fix: Added dependency array for react native web project (crashing react native reanimated 3.X) Jul 26, 2023

agusvazquez mentioned this pull request Jul 27, 2023

iOS: Version 2.17.0 fails to build on React Native 0.72.0+ software-mansion/react-native-reanimated#4522

Closed

Merge branch 'main' into main

74f3876

Added some missing properties on react native reanimated hooks

57f1f02

Merge branch 'main' into main

8066586

tjzel suggested changes Aug 1, 2023

View reviewed changes

Fixed feedback

e0c8083

tjzel approved these changes Aug 8, 2023

View reviewed changes

satya164 requested changes Sep 20, 2023

View reviewed changes

Added additionalHooks

46291c5

satya164 added the needs response label Sep 25, 2023

github-actions bot added the stale label Oct 26, 2023

github-actions bot closed this Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added dependency array for react native web project (crashing react native reanimated 3.X) #11496

fix: Added dependency array for react native web project (crashing react native reanimated 3.X) #11496

agusvazquez commented Jul 25, 2023 •

edited

Loading

github-actions bot commented Jul 25, 2023

netlify bot commented Jul 25, 2023 •

edited

Loading

agusvazquez commented Jul 27, 2023

radelcom commented Jul 31, 2023

agusvazquez commented Jul 31, 2023 •

edited

Loading

codecov-commenter commented Aug 1, 2023 •

edited by codecov bot

Loading

tjzel commented Aug 1, 2023

agusvazquez commented Aug 1, 2023

tjzel commented Aug 1, 2023

tjzel Aug 1, 2023

agusvazquez Aug 4, 2023

tjzel Aug 8, 2023

agusvazquez Aug 8, 2023

tjzel Aug 8, 2023

tjzel Aug 1, 2023

tjzel Aug 1, 2023

tjzel Aug 1, 2023

tjzel Aug 1, 2023

tjzel Aug 1, 2023

tjzel commented Aug 1, 2023

agusvazquez commented Aug 4, 2023

sacru2red commented Sep 4, 2023 •

edited

Loading

satya164 left a comment

agusvazquez commented Sep 20, 2023

satya164 commented Sep 20, 2023

github-actions bot commented Oct 26, 2023

fix: Added dependency array for react native web project (crashing react native reanimated 3.X) #11496

fix: Added dependency array for react native web project (crashing react native reanimated 3.X) #11496

Conversation

agusvazquez commented Jul 25, 2023 • edited Loading

github-actions bot commented Jul 25, 2023

netlify bot commented Jul 25, 2023 • edited Loading

✅ Deploy Preview for react-navigation-example ready!

agusvazquez commented Jul 27, 2023

radelcom commented Jul 31, 2023

agusvazquez commented Jul 31, 2023 • edited Loading

codecov-commenter commented Aug 1, 2023 • edited by codecov bot Loading

Codecov Report

tjzel commented Aug 1, 2023

agusvazquez commented Aug 1, 2023

tjzel commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjzel commented Aug 1, 2023

agusvazquez commented Aug 4, 2023

sacru2red commented Sep 4, 2023 • edited Loading

satya164 left a comment

Choose a reason for hiding this comment

agusvazquez commented Sep 20, 2023

satya164 commented Sep 20, 2023

github-actions bot commented Oct 26, 2023

agusvazquez commented Jul 25, 2023 •

edited

Loading

netlify bot commented Jul 25, 2023 •

edited

Loading

agusvazquez commented Jul 31, 2023 •

edited

Loading

codecov-commenter commented Aug 1, 2023 •

edited by codecov bot

Loading

sacru2red commented Sep 4, 2023 •

edited

Loading