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

[Performance] Configure Reanimated to Version >2 for FPS boost #3972

Closed
parasharrajat opened this issue Jul 10, 2021 · 17 comments
Closed

[Performance] Configure Reanimated to Version >2 for FPS boost #3972

parasharrajat opened this issue Jul 10, 2021 · 17 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@parasharrajat
Copy link
Member

parasharrajat commented Jul 10, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


We upgraded to React-navigation v6 but we are still lacking few gains from it. It supports Reanimated v2 which is proposed to be better and performant in every way. If we are getting smooth animations from it, then I believe that we should upgrade.

Expected Result:

Existing animation should not break.

Actual Result:

We see a warning for it #2180 (comment).
image

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
NONE

Platform:

Where is this issue occurring?

Web ✅
iOS ✅
Android ✅
Desktop App ✅
Mobile Web ✅

Version Number: latest
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@parasharrajat parasharrajat added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 10, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 10, 2021
@MelvinBot
Copy link

Triggered auto assignment to @MonilBhavsar (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@trjExpensify
Copy link
Contributor

Going to hand this over to someone in engineering to explore further, @parasharrajat. If there has been a wider discussion about this update somewhere in Slack, feel free to include it in the issue.

@marcaaron
Copy link
Contributor

which is proposed to be better and performant in every way. If we are getting smooth animations from it, then I believe that we should upgrade

Sounds good. But before moving forward with the change I think we should do some benchmarks or produce evidence that performance will actually improved by this change. There are a million and one things we can do to improve performance. Figuring out which ones are worth doing is the challenge.

@marcaaron marcaaron changed the title Update Reanimated to Version >2 for FPS boost [Performance] Update Reanimated to Version >2 for FPS boost Jul 14, 2021
@jsamr
Copy link
Collaborator

jsamr commented Jul 14, 2021

react-native-reanimated is already in version 2.1.0 on master

https://github.com/Expensify/Expensify.cash/blob/bb5ae3e99ca3ea85a0dc7505429981f5756699e0/package.json#L89

That is why I found this warning in #4014 hard to explain...

@parasharrajat
Copy link
Member Author

@jsamr But we need to set up it in the Native files so that it can be used.

  1. For Android, mainApplication.java (Instructions https://docs.swmansion.com/react-native-reanimated/docs/installation#android)
  2. Web requires webpack plugin.
  3. IOS just works with pod install.

@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 14, 2021

I have prepared all the changes just need a 🟢 to create a PR. But Marc has a good point. It is only going to affect the slide animation of the Drawer which currently works just fine.

@jsamr
Copy link
Collaborator

jsamr commented Jul 14, 2021

@parasharrajat Sorry you are right, I didn't mean to be presumptuous! I had seen the babel plugin set up and had assumed it was already configured:

https://github.com/Expensify/Expensify.cash/blob/bb5ae3e99ca3ea85a0dc7505429981f5756699e0/babel.config.js#L30

@marcaaron
I had a great experience with react-native-reanimated@^2 with expo. JSI showing its amazing potential, really beautiful, performant animations in the UI thread! So this change will most certainly not address any of the performance issue discussed on Slack yesterday. It has the potential to make any non-native animation from react-navigation smoother because it being run on the UI thread (vs JS thread in v1). react-navigation drawer component peer-depends on V1+

https://github.com/react-navigation/react-navigation/blob/a70adfbca1cfc3141a6dffb99e685563a7d52a08/packages/drawer/package.json#L68

I guess the best course of action is chose between v2 and v1 and stick with it, instead of using v2 in what seems to be a legacy or fallback mode and thus triggering this warning.

@marcaaron
Copy link
Contributor

Sounds like this is worthwhile to do so I think we should move forward here. I don't expect this to solve any major issues, but does sound like a good direction to move in and ultimately iOS and Android should be using the same features. To clarify, will some change be required to react-navigation drawer as well?

@marcaaron marcaaron added the External Added to denote the issue can be worked on by a contributor label Jul 15, 2021
@MelvinBot
Copy link

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@marcaaron
Copy link
Contributor

Conversation in Slack ongoing, but excited to see this change.

@parasharrajat
Copy link
Member Author

To clarify, will some change be required to the react-navigation drawer as well?

None.

@parasharrajat parasharrajat changed the title [Performance] Update Reanimated to Version >2 for FPS boost [Performance] Configure Reanimated to Version >2 for FPS boost Jul 15, 2021
@NicMendonca
Copy link
Contributor

@parasharrajat created a job for this in Upwork - https://www.upwork.com/jobs/~01583ab581297f02f0

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 15, 2021
@MelvinBot
Copy link

Triggered auto assignment to @roryabraham (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@roryabraham
Copy link
Contributor

Okay, looks like there's not a ton for me to review here in terms of the proposal @parasharrajat, see you in the PR!

@NicMendonca NicMendonca added Weekly KSv2 and removed Daily KSv2 labels Jul 19, 2021
@parasharrajat
Copy link
Member Author

I think this can be closed and the Help Wanted label can be remvoed as well.

@parasharrajat
Copy link
Member Author

@NicMendonca I think this can be closed and the Help Wanted label can be removed as well.

@mallenexpensify mallenexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants