perf: reanimated 3 upgrade #11567

hannojg · 2022-10-04T10:18:41Z

Details

This PR upgrades reanimated to version 3. It's based on #11550 by @marcaaron since his PR removes any reanimated v1 usage (which rea v3 dropped support for). [This PR has since landed on main, so that's good!]

This PR is an effort toward enabling fabric architecture.

Fixed Issues

Related #8503

Tests

Crop and edit your avatar
Use the left hand nav bar to navigate to reports

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Crop and edit your avatar
Use the left hand nav bar to navigate to reports

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

…f/rea-3-upgrade

hannojg · 2022-10-04T13:45:06Z

Update:

After talking to the team at react-navigation it appears that we have to use force install, or wait until reanimated v3 is officially released:
react-navigation/react-navigation#10909 (comment)

… or we switch to using yarn, which is faster anyways

melvin-bot · 2022-10-10T21:54:53Z

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

melvin-bot · 2022-10-10T21:54:56Z

Looks like you modified DeprecatedCustomActions.js! To be clear, you should not be adding any code to this file.

This file contains deprecated workarounds for limitations of react-navigation and a more long term solution will need to be investigated.

Drop a note in #expensify-open-source to discuss with the team how to proceed.

…f/rea-3-upgrade

hannojg · 2022-10-17T11:47:51Z

I just added a fix with patch-package for the installation issue, which we can use until REA v3 is officially released.
Are we cool with that?

roryabraham · 2022-10-18T17:33:49Z

Yes, using patch-package is fine 👍🏼 Let's get this PR polished up and over the finish line

Santhosh-Sellavel · 2022-10-19T19:31:37Z

@hannojg Are there any test steps here, Does something need to compare?

cc: @Beamanator

This reverts commit d8a07bf.

hannojg · 2022-10-20T05:50:39Z

@Santhosh-Sellavel this is just some internal library update. Reanimated isn't used too much around the code base. Most noticeably it's used for the drawer (reports list/ SideBarLinks and the actual Report View). So any interaction with the drawer maybe should be paid attention to.

However, really opening the app and it not crashing should be test-steps enough for this change 😄

JmillsExpensify · 2022-10-25T02:19:43Z

Are you all coming back to this PR today? Looks like we're at the finish line, just need to resolve the console errors. Right?

hannojg · 2022-10-25T06:00:46Z

Yes, gonna look into it again today so we can merge it today!

hannojg · 2022-10-25T08:13:45Z

Adding to my testing notes:

Warnings are reproducible when navigating from list to report, so its very likely coming from the drawer's REA usage

hannojg · 2022-10-25T08:28:45Z

Okay everyone, we can safely ignore this warning. For two reasons:

It's just a developer warning that react native generated for us to let us know we can reduce sending events to the JS side, as no listeners for native events are set up yet. In REA 3 they forget a check for that, I will make a PR to get rid of that warning (however this doesn't have to block us from merging IMO (but not sure how you guys treat new warnings in the console))
Most importantly: once we turn on the new arch, the code in REA that sends these events will be handled differently anyways, so we are not getting hit by those events:

I will look into the web warnings shortly, but just quickly looking at it it just seems like a general warning that we can't really do anything about. We are not using the legacy code, however, the warnings are still generated just by the pure presence of the legacy code. I think this can/will only be resolved when the legacy implementation gets removed from the drawer lib. But maybe we can at least suppress these warnings if we want to.

hannojg · 2022-10-25T09:09:15Z

REA issue and accompanying PR is up: software-mansion/react-native-reanimated#3716

hannojg · 2022-10-25T11:53:24Z

We can also ignore the web warnings. If you want to I can add a commit to suppress the warnings. Can only go away once the legacy code gets removed :)

So as a summary I think we are good to merge

software-mansion/react-native-reanimated#3717

roryabraham · 2022-10-25T13:57:12Z

Created this issue to follow-up and remove the patch

roryabraham · 2022-10-25T15:16:13Z

Sorry @hannojg, I think you need to reinstall pods and commit the diff

software-mansion/react-native-reanimated#3701 Only applied parts, because on REA's main branch the ios project setup changed a bit so i needed to only use whats necessary for the current setup.\ This part of the patch can be reverted once the PR landed and we updated the REA version

Santhosh-Sellavel · 2022-10-25T17:32:09Z

I am seeing changes committed frequently. @hannojg is this ready for review?

AndrewGable · 2022-10-25T17:34:12Z

Yes it is, it has some priority so if you could review ASAP @Santhosh-Sellavel that would be great!

AndrewGable

Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

@Santhosh-Sellavel - Let me know when you've approved and I can merge 🚀

Santhosh-Sellavel · 2022-10-25T17:58:36Z

@AndrewGable We are Ignoring the console errors, right?

AndrewGable · 2022-10-25T18:03:09Z

From reading #11567 (comment) - Yes, I believe we are OK to ignore warnings.

Santhosh-Sellavel · 2022-10-25T18:29:23Z

Screen Recording

Desktop

Screen.Recording.2022-10-25.at.11.27.37.PM.mov

Web

Screen.Recording.2022-10-25.at.11.32.06.PM.mov

iOS (Build using Xcode)

Simulator.Screen.Recording.-.iPhone.13.-.2022-10-25.at.23.20.01.mp4

Android

Screen.Recording.2022-10-25.at.11.20.31.PM.mov

mWeb

Simulator.Screen.Recording.-.iPhone.13.-.2022-10-25.at.23.35.38.mp4

PR Reviewer Checklist

C+ Reviewed
🎀 👀 🎀

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Santhosh-Sellavel · 2022-10-25T18:30:50Z

@AndrewGable I am having trouble while running yarn ios, build always fails does that work for you? I've used Xcode app to build the app.

AndrewGable · 2022-10-25T18:50:24Z

I am able to successfully build via npm run ios

Santhosh-Sellavel · 2022-10-25T18:51:35Z

Thanks then we are good to merge!

Santhosh-Sellavel

#11567 (comment)

Santhosh-Sellavel · 2022-10-25T20:18:50Z

iOS build working now after npm run clean

iOS

Simulator.Screen.Recording.-.iPhone.13.-.2022-10-26.at.01.46.39.mp4

OSBotify · 2022-10-25T20:25:17Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-10-26T23:20:07Z

🚀 Deployed to staging by @AndrewGable in version: 1.2.20-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-10-28T02:24:49Z

🚀 Deployed to production by @chiragsalian in version: 1.2.20-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

hannojg added 4 commits October 4, 2022 09:51

bump REA package

45fe329

android: remove obsolete REA configuration

05f07b5

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

a98d347

…f/rea-3-upgrade

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

8a38df0

…f/rea-3-upgrade

hannojg mentioned this pull request Oct 4, 2022

Enable Modern Drawer in react-navigation #11550

Merged

91 tasks

hannojg force-pushed the perf/rea-3-upgrade branch 2 times, most recently from b32cb9c to 8a38df0 Compare October 4, 2022 13:43

JmillsExpensify changed the title ~~perf: reanimated 3 upgrade~~ [HOLD] perf: reanimated 3 upgrade Oct 10, 2022

hannojg added 3 commits October 14, 2022 06:56

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

ff87db8

…f/rea-3-upgrade

pod install

d8a07bf

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

5631d44

…f/rea-3-upgrade

temp: patch package to fix reanimated v3-rc installation issue

f87600e

hannojg force-pushed the perf/rea-3-upgrade branch from 25cf51c to f87600e Compare October 17, 2022 11:51

JmillsExpensify changed the title ~~[HOLD] perf: reanimated 3 upgrade~~ perf: reanimated 3 upgrade Oct 18, 2022

trjExpensify mentioned this pull request Oct 18, 2022

[HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture #8503

Closed

38 tasks

fix tests

18e1ebc

hannojg marked this pull request as ready for review October 19, 2022 07:48

hannojg requested a review from a team as a code owner October 19, 2022 07:48

melvin-bot bot requested review from Beamanator and Santhosh-Sellavel and removed request for a team October 19, 2022 07:48

AndrewGable self-requested a review October 19, 2022 14:44

Revert "pod install"

1d78276

This reverts commit d8a07bf.

hannojg added 2 commits October 25, 2022 15:34

add REA patch, see:

f19efc7

software-mansion/react-native-reanimated#3717

run patch-package in post install

8ae7ddf

hannojg added 4 commits October 25, 2022 17:22

update package-lock file

1ea7172

update Podfile.lock

20e0fdd

updated podspec after patch

06d91fc

AndrewGable approved these changes Oct 25, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Oct 25, 2022

View reviewed changes

AndrewGable merged commit 0973e75 into Expensify:main Oct 25, 2022

OSBotify mentioned this pull request Oct 26, 2022

Deploy Checklist: New Expensify 2022-10-26 #12165

Closed

28 tasks

perf: reanimated 3 upgrade #11567

perf: reanimated 3 upgrade #11567

Conversation

hannojg commented Oct 4, 2022 • edited by AndrewGable Loading

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

hannojg commented Oct 4, 2022 • edited Loading

melvin-bot bot commented Oct 10, 2022

melvin-bot bot commented Oct 10, 2022

hannojg commented Oct 17, 2022 • edited Loading

roryabraham commented Oct 18, 2022

Santhosh-Sellavel commented Oct 19, 2022

hannojg commented Oct 20, 2022

JmillsExpensify commented Oct 25, 2022

hannojg commented Oct 25, 2022

hannojg commented Oct 25, 2022

hannojg commented Oct 25, 2022 • edited Loading

hannojg commented Oct 25, 2022

hannojg commented Oct 25, 2022

roryabraham commented Oct 25, 2022

roryabraham commented Oct 25, 2022

Santhosh-Sellavel commented Oct 25, 2022

AndrewGable commented Oct 25, 2022

AndrewGable left a comment • edited Loading

Choose a reason for hiding this comment

Checklist

Screenshots

Santhosh-Sellavel commented Oct 25, 2022

AndrewGable commented Oct 25, 2022

Santhosh-Sellavel commented Oct 25, 2022 • edited Loading

Desktop

Web

iOS (Build using Xcode)

Android

mWeb

PR Reviewer Checklist

Santhosh-Sellavel commented Oct 25, 2022

AndrewGable commented Oct 25, 2022

Santhosh-Sellavel commented Oct 25, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Oct 25, 2022

iOS

OSBotify commented Oct 25, 2022

OSBotify commented Oct 26, 2022

OSBotify commented Oct 28, 2022

hannojg commented Oct 4, 2022 •

edited by AndrewGable

Loading

hannojg commented Oct 4, 2022 •

edited

Loading

hannojg commented Oct 17, 2022 •

edited

Loading

hannojg commented Oct 25, 2022 •

edited

Loading

AndrewGable left a comment •

edited

Loading

Santhosh-Sellavel commented Oct 25, 2022 •

edited

Loading