[No QA] Upgrade RN to 0.70.4 (aka 0.70.1) #10860

roryabraham · 2022-09-07T10:37:27Z

How to review this PR

A lot of these changes might seem kind of random, but I was simply following the RN upgrade helper between our last version (0.69.3) and the new one (0.70.1): https://react-native-community.github.io/upgrade-helper/?from=0.69.3&to=0.70.1

Use that as a guide to review these changes.

Details

This PR update the React Native version to @expensify/react-native@0.70.2, which is based on React Native 0.70.1. Hopefully the next time we publish the fork, it will be with the same version from the upstream. My bad!

This upgrade should result in much better JSI performance, hopefully will fix the debug-build performance issues on the Android app. Overall many of the other upgrades from 0.70 are config or build-system related AFAICT.

As part of this, I had to update reanimated to a version that's compatible with RN 0.70.

Fixed Issues

(partial) #8503
(partial) #10345

Tests

Run the app. Make sure that the build succeeds and perform various regression tests of basic functionality to see that it works as expected.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

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

QA Steps

None, regression tests only.

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

roryabraham · 2022-09-23T00:34:11Z

Took me a bit but this is ready for review!

marcaaron

✅ - Web
✅ - Desktop
✅ - iOS
🟨 - Android dev builds - seem 1000x better
- Navigation is a bit slow / unusable on my account with a few hundred chats
- But I could not even test the app locally before so huge improvement overall
🟥 - Android release build didn't work for me and I ran into this weird error

Execution failed for task ':app:bundleReleaseJsAndAssets'.
> java.lang.Exception: Couldn't determine Hermesc location. Please set `project.ext.react.hermesCommand` to the path of the hermesc binary file. node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc

Tried to look at that path but didn't find hermesc in node_modules/react-native/sdks.

Julesssss · 2022-09-23T12:53:00Z

android/app/build.gradle

-            // We configure the NDK build only if you decide to opt-in for the New Architecture.
+            // We configure the CMake build only if you decide to opt-in for the New Architecture.


It looks like we're switching from the ndk to Cmake? In which case, can we remove the other ndk config?

Well we're switching from ndk-build to CMake, but the Java NDK is still used ... that's what enables Java and C++ code to communicate with eachother

Julesssss · 2022-09-23T12:53:24Z

android/app/build.gradle

@@ -189,8 +182,8 @@ android {
    if (isNewArchitectureEnabled()) {
        // We configure the NDK build only if you decide to opt-in for the New Architecture.


We configure the CMake build

Julesssss

Web and Desktop worked for me 👍

iOS - I'm unable to build due to this error, even after pod deintegrate. Are you guys using a specific version of XCode?

Android - I see the same issue as Marc, but interestingly it also occurs on main... Here's the RN issue, many people seem to be having the same problem

roryabraham · 2022-09-23T14:33:40Z

iOS - I'm unable to build due to this error, even after pod deintegrate

@Julesssss Make sure you pull changes? It should be:

use_react_native!(
+    :path => config[:reactNativePath],
-    :path => config["reactNativePath"],
    ...

Julesssss

I found a workaround for the Android Release build issue. Simply generate a release build manually with Build > Generate Signed bundle / APK. Android release and debug builds are testing well 👍

Make sure you pull changes? It should be:

Ah, thanks. I had switched to main for testing and forgot I was on the wrong branch 🤦 I'm now seeing a different error. Trying a full pod deintegrate again...

roryabraham · 2022-09-23T14:54:55Z

I'm now seeing a different error

I saw that error and fixed it with pod update RCT-Folly --no-repo-update and committed the changes. Shouldn't have to do that again 🤔

roryabraham · 2022-09-23T14:58:26Z

I found a workaround for the Android Release build issue

Hmmm ... I think node_modules/react-native/sdks/hermesc should contain some prebuilt hermes binaries, but that's not happening. I might need to either republish or create a workaround that downloads those binaries in a postinstall script.

roryabraham · 2022-09-23T15:12:06Z

Going to republish once again ☹️

roryabraham · 2022-09-23T16:08:54Z

Updated and retested – this now appears to be working on Web, Desktop, iOS, Android (debug) AND Android (release) 👍🏼

Thanks @marcaaron for being super diligent and testing a release build too 👏🏼

FWIW, I did what I had done in the past and downloaded the pre-built Hermes binaries, force-committed them in the fork, and included them in the release. This is ideal because it means that nobody (not even our GitHub CI/CD) has to compile Hermes, which takes like an hour

marcaaron

LGTM and works great !

marcaaron · 2022-09-23T17:41:26Z

Are you guys using a specific version of XCode?

@Julesssss I'm on 13.4.1 atm. I did have to run some pod commands that were recommended to update the hermes build there

marcaaron · 2022-09-23T17:42:13Z

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 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 tagging the marketing team 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 */
- Any functional components have the displayName property
- 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.

marcaaron · 2022-09-23T17:58:56Z

Gonna merge this so we can keep up the #urgency on WhatsApp Quality.

OSBotify · 2022-09-23T18:03:57Z

✋ 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-09-23T23:32:44Z

🚀 Deployed to staging by @marcaaron in version: 1.2.6-0 🚀

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

Julesssss

👍

OSBotify · 2022-09-26T20:19:40Z

🚀 Deployed to production by @roryabraham in version: 1.2.6-0 🚀

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

Upgrade RN to 0.70.1

0900616

roryabraham changed the title ~~Upgrade RN to 0.70.1~~ [WIP] Upgrade RN to 0.70.1 Sep 7, 2022

roryabraham added 2 commits September 7, 2022 15:01

Bump react-test-renderer to 18.2.0

32125f8

Rename loadLibrary call

5111caa

trjExpensify mentioned this pull request Sep 13, 2022

[Tracking] [Performance] Margelo/Expensify - Performance Improvements #10962

Closed

roryabraham added 3 commits September 21, 2022 17:35

Merge branch 'main' into Rory-UpgradeRN

e003fac

Update RN to @expensify/react-native@0.70.2

0461aed

Update xcproj

e3a69b2

roryabraham changed the title ~~[WIP] Upgrade RN to 0.70.1~~ [WIP] Upgrade RN to 0.70.2 (aka 0.70.1) Sep 22, 2022

roryabraham mentioned this pull request Sep 22, 2022

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

Closed

38 tasks

Bump version to 0.70.3 and install pods

12b7bda

roryabraham changed the title ~~[WIP] Upgrade RN to 0.70.2 (aka 0.70.1)~~ [WIP] Upgrade RN to 0.70.3 (aka 0.70.1) Sep 22, 2022

roryabraham self-assigned this Sep 22, 2022

Merge branch 'main' into Rory-UpgradeRN

dfb7adf

This comment was marked as outdated.

Sign in to view

Bump reanimated to fix android crash

386ef8b

roryabraham changed the title ~~[WIP] Upgrade RN to 0.70.3 (aka 0.70.1)~~ [WIP][No QA] Upgrade RN to 0.70.3 (aka 0.70.1) Sep 23, 2022

Update reanimated pod

ead43fd

roryabraham changed the title ~~[WIP][No QA] Upgrade RN to 0.70.3 (aka 0.70.1)~~ [No QA] Upgrade RN to 0.70.3 (aka 0.70.1) Sep 23, 2022

roryabraham marked this pull request as ready for review September 23, 2022 00:33

roryabraham requested a review from a team as a code owner September 23, 2022 00:33

melvin-bot bot requested review from dangrous and removed request for a team September 23, 2022 00:34

roryabraham requested a review from Julesssss September 23, 2022 00:35

marcaaron reviewed Sep 23, 2022

View reviewed changes

Julesssss reviewed Sep 23, 2022

View reviewed changes

Update RN fork to 0.70.4

b4cd378

roryabraham requested review from marcaaron and Julesssss September 23, 2022 16:12

roryabraham changed the title ~~[No QA] Upgrade RN to 0.70.3 (aka 0.70.1)~~ [No QA] Upgrade RN to 0.70.4 (aka 0.70.1) Sep 23, 2022

marcaaron approved these changes Sep 23, 2022

View reviewed changes

marcaaron merged commit 5898727 into main Sep 23, 2022

marcaaron deleted the Rory-UpgradeRN branch September 23, 2022 17:59

OSBotify mentioned this pull request Sep 23, 2022

Deploy Checklist: New Expensify 2022-09-23 #11252

Closed

14 tasks

roryabraham mentioned this pull request Sep 23, 2022

[$1000] DEV Env - App performance on Android is extremely slow - reported by @parasharrajat #10345

Closed

parasharrajat mentioned this pull request Sep 24, 2022

[$16,000] Let's fix a React Native issue -- useWindowDimensions() returns swapped height and width in iOS #2727

Closed

5 tasks

Julesssss reviewed Sep 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[No QA] Upgrade RN to 0.70.4 (aka 0.70.1) #10860

[No QA] Upgrade RN to 0.70.4 (aka 0.70.1) #10860

roryabraham commented Sep 7, 2022 •

edited by marcaaron

Loading

This comment was marked as outdated.

roryabraham commented Sep 23, 2022

marcaaron left a comment

Julesssss Sep 23, 2022

roryabraham Sep 23, 2022

Julesssss Sep 23, 2022

Julesssss left a comment

roryabraham commented Sep 23, 2022

Julesssss left a comment •

edited

Loading

roryabraham commented Sep 23, 2022

roryabraham commented Sep 23, 2022

roryabraham commented Sep 23, 2022

roryabraham commented Sep 23, 2022 •

edited

Loading

marcaaron left a comment

marcaaron commented Sep 23, 2022

marcaaron commented Sep 23, 2022

marcaaron commented Sep 23, 2022

OSBotify commented Sep 23, 2022

OSBotify commented Sep 23, 2022

Julesssss left a comment

OSBotify commented Sep 26, 2022

		// We configure the NDK build only if you decide to opt-in for the New Architecture.
		// We configure the CMake build only if you decide to opt-in for the New Architecture.

		@@ -189,8 +182,8 @@ android {
		if (isNewArchitectureEnabled()) {
		// We configure the NDK build only if you decide to opt-in for the New Architecture.

[No QA] Upgrade RN to 0.70.4 (aka 0.70.1) #10860

[No QA] Upgrade RN to 0.70.4 (aka 0.70.1) #10860

Conversation

roryabraham commented Sep 7, 2022 • edited by marcaaron Loading

How to review this PR

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

This comment was marked as outdated.

roryabraham commented Sep 23, 2022

marcaaron left a comment

Choose a reason for hiding this comment

Julesssss Sep 23, 2022

Choose a reason for hiding this comment

roryabraham Sep 23, 2022

Choose a reason for hiding this comment

Julesssss Sep 23, 2022

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

roryabraham commented Sep 23, 2022

Julesssss left a comment • edited Loading

Choose a reason for hiding this comment

roryabraham commented Sep 23, 2022

roryabraham commented Sep 23, 2022

roryabraham commented Sep 23, 2022

roryabraham commented Sep 23, 2022 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Sep 23, 2022

marcaaron commented Sep 23, 2022

marcaaron commented Sep 23, 2022

OSBotify commented Sep 23, 2022

OSBotify commented Sep 23, 2022

Julesssss left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 26, 2022

roryabraham commented Sep 7, 2022 •

edited by marcaaron

Loading

Julesssss left a comment •

edited

Loading

roryabraham commented Sep 23, 2022 •

edited

Loading