search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.35k stars 2.77k forks source link

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

Closed parasharrajat closed 3 years ago

parasharrajat commented 3 years ago

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 https://github.com/Expensify/Expensify.cash/issues/2180#issuecomment-861909818. 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 :white_check_mark: iOS :white_check_mark: Android :white_check_mark: Desktop App :white_check_mark: Mobile Web :white_check_mark:

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

MelvinBot commented 3 years ago

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

MelvinBot commented 3 years ago

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

trjExpensify commented 3 years ago

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 commented 3 years ago

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.

jsamr commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

I have prepared all the changes just need a :green_circle: 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 commented 3 years ago

@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 commented 3 years ago

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?

MelvinBot commented 3 years ago

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

marcaaron commented 3 years ago

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

parasharrajat commented 3 years ago

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

None.

NicMendonca commented 3 years ago

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

MelvinBot commented 3 years ago

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

roryabraham commented 3 years ago

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

parasharrajat commented 3 years ago

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

parasharrajat commented 3 years ago

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