Update App to React Native 0.72 version

[fabioh8010]

Details

This PR aims to update Expensify's App to use our RN 0.72 fork. Some changes were made in order to follow upgrading from previous RN version to current one, following this steps.

SLACK DISCUSSION THREAD: https://expensify.slack.com/archives/C01GTK53T8Q/p1683321604215369

Patches created and corresponding upstream PRs

• @onfido/react-native-sdk -> https://github.com/onfido/react-native-sdk/pull/130
• lottie-react-native -> https://github.com/lottie-react-native/lottie-react-native/issues/1022
• @oguzhnatly/react-native-image-manipulator -> We are going to replace with Expo Image Manipulator in the future

Fixed Issues

$ https://github.com/Expensify/App/issues/18444 $ https://github.com/Expensify/App/issues/21308 PROPOSAL: -

Tests

Full regression test

• [x] Verify that no errors appear in the JS console

Offline tests

Same as above.

QA Steps

Same as above.

• [x] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] 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)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android / native
  • [x] Android / Chrome
  • [x] iOS / native
  • [x] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] 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)
  • [x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] 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.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • [x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] 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.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] 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
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] 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)
  • [x] 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)
  • [x] All JSX used for rendering exists in the render method
  • [x] 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
• [x] If any new file was added I verified that:
  • [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
• [x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• [x] 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)
• [x] 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.
• [x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• [x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

[fabioh8010]

@roryabraham Your steps solved the problem. Strangely, node_modules was not present in that folder and I had to install it manually like you said. Is it something that we have to do manually or should it install together when running npm install on the root folder? I'm asking this because I don't remember having to do it before.

[fabioh8010]

Update

• All builds are stable ✅
• Documented the patches and PRs in the issue description.
• Addressed some comments here.

@mountiny @roryabraham I think we are good to go with this PR now (we have one pending discussion here though). Please review the PR and let me know if you need more testing from my side, or a specific one.

[roryabraham]

Is it something that we have to do manually or should it install together when running npm install on the root folder? I'm asking this because I don't remember having to do it before.

It should happen in a postInstall script

[roryabraham]

@fabioh8010 there's an unrelated test failure that was present on main. Can you merge main again to resolve that please?

[parasharrajat]

Awesome timing @fabioh8010. Going to run a full App scan...

[parasharrajat]

The web is looking pretty unchanged(Good).

[parasharrajat]

Android Looks good.

[parasharrajat]

Every time, I close the keyboard On iOS, I see a warning.

[fabioh8010]

Hi @parasharrajat Yes, I saw this warning as well and I was able to reproduce it on main on Friday. I will test it again.

[parasharrajat]

Question: Do you see opening or closing animation for the FAB menu on iOS and Android?

[ntdiary]

Every time, I close the keyboard On iOS, I see a warning.

https://github.com/Expensify/App/blob/b7004b9e9ff2bb86d6357e62ce15659322032c65/patches/react-native%2B0.71.2-alpha.3.patch#L67

@parasharrajat, this issue is that enabled is not defined, I guess we wanted to use this.props.enabled here?

For this PR, it's react-native+0.72.1.patch

[fabioh8010]

Makes sense @ntdiary, thanks for investigation! This is probably happening on main as well because this patch is also there, @parasharrajat could you please confirm this?

[parasharrajat]

Yes, it is present on the main as well. So let's not fix it here. Reported https://expensify.slack.com/archives/C049HHMV9SM/p1688406249179949.

[fabioh8010]

Question: Do you see opening or closing animation for the FAB menu on iOS and Android?

Testing..

[fabioh8010]

@parasharrajat I tested the FAB button on both main and this branch and there is no difference in my machine.

"main" branch - Android

https://github.com/Expensify/App/assets/20051562/9df56ac7-e923-4fa3-95fd-183852fec3dc

"main" branch - iOS

https://github.com/Expensify/App/assets/20051562/a86cfdfa-fb85-4d2c-9c8b-ac44314e55e6

this branch - Android

https://github.com/Expensify/App/assets/20051562/d8b3cf5e-969b-4765-81d4-b51b9f2912c9

this branch - iOS

https://github.com/Expensify/App/assets/20051562/7543a0cc-be5a-44e6-8ad1-05fbdfc47a99

Besides that, I've updated this branch against main again, please feel free to continue testing.

[parasharrajat]

Thanks. It might be just due to heavy dev builds. I am trying to find bugs because it is hard to believe that it worked without a hitch.

[fabioh8010]

@parasharrajat Please let me know if you need more help from my side.

[parasharrajat]

Yeah, sure. I will finish my testing today.

[aimane-chnaif]

We should also check all other features and bug fixes done from our forked RN, which not existed in upstream RN. As this PR will revert those which are not applied to upstream RN.

[parasharrajat]

Good call @aimane-chnaif

[mountiny]

Good call, I thought we have already managed to get all our fork changes to the upstream @chrispader would you be able to have a look at this?

If this would require a patch while we will wait for the fix to hit upstream, I think that would be better than holding this update.

[fabioh8010]

Good call, I thought we have already managed to get all our fork changes to the upstream @chrispader would you be able to have a look at this?

@mountiny @aimane-chnaif I recalled about a spreadsheet with the status of each PR merged in Expensify's fork that was shared in the channel -> https://docs.google.com/spreadsheets/d/1Fvc2x0ythHoqE1NnsP5JTOLC0eXbWV5IrA-37hbJDGc/edit#gid=1594942597

Tomorrow I can take a look in each PR and see if's actually available on the last release.

If this would require a patch while we will wait for the fix to hit upstream, I think that would be better than holding this update.

It's worth mentioning that it won't be possible to use patches to fix native issues in Android, as the Android sources are now shipped in pre-built artifacts. cc @chrispader

[fabioh8010]

Looking through the spreadsheet and changelog:

🟢 Merged and available on Upstream RN. 🟡 Merged but not available on latest RN release yet. 🟠 Not sure about status on Upstream RN. 🔴 No Upstream PR or not merged on Upstream RN yet. 🟣 Patched on this PR. ❌ Possible blocker. 🔨 Can be patched if necessary.

• 🟡 / 🔨 https://github.com/Expensify/react-native/pull/53: Merged on our Fork. Merged on Upstream but not available in the latest release yet. Related issue.

• 🔴 https://github.com/Expensify/react-native/pull/49: Merged on our Fork. Still open on Upstream. Related issue.

• 🟢 https://github.com/Expensify/react-native/pull/46: Merged on our Fork. This one and this one available since 0.72.0.

• 🔴 https://github.com/Expensify/react-native/pull/45: Merged on our Fork. Still open on Upstream. Related issue.

• 🟡 / 🔨 https://github.com/Expensify/react-native/pull/40: Merged on our Fork. Merged on Upstream but not available in the latest release yet. Related issue.

• 🟣 https://github.com/Expensify/react-native/pull/37: Merged on our Fork. Still open on Upstream. Related issue. Patched with Upstream PR.

• 🟢 https://github.com/Expensify/react-native/pull/36: Merged on our Fork. Merged on Upstream and available since 0.72.0.

• 🟣 / 🟢 https://github.com/Expensify/react-native/pull/33: Merged on our Fork. Two PRs merged on Upstream, here and here, and available since 0.72.4. Related issue. Patched with Fork PR.

• 🟢 https://github.com/Expensify/react-native/pull/32: Merged on our Fork. Merged on Upstream and available since 0.72.0.

• 🟢 https://github.com/Expensify/react-native/pull/24: Merged on our Fork. Merged on Upstream and available since 0.70.1.

• 🟢 https://github.com/Expensify/react-native/pull/21: Merged on our Fork. It's just examples for RNTester.

• 🟢 https://github.com/Expensify/react-native/pull/18: Merged on our Fork. Merged on Upstream and available since 0.72.0.

• 🟢 https://github.com/Expensify/react-native/pull/17: Merged on our Fork. Merged on Upstream and available since 0.72.0.

• 🔴 / 🔨 https://github.com/Expensify/react-native/pull/16: Merged on our Fork. Still open on Upstream.

• 🟢 https://github.com/Expensify/react-native/pull/11: Merged on our Fork. Merged on Upstream and available since 0.70.0.

• 🟢 https://github.com/Expensify/react-native/pull/8: Merged on our Fork. Merged on Upstream and available since 0.72.0.

• 🟢 https://github.com/Expensify/react-native/pull/55: Merged on our Fork. Merged on Upstream and available since 0.72.0.

cc @mountiny

[parasharrajat]

Hmm, There are a lot with undefined status. I didn't know there was a sheet. Thanks.

• https://github.com/Expensify/react-native/pull/11 : Looks merged https://github.com/facebook/react-native/pull/34014

• https://github.com/facebook/react-native/pull/37424 - Missing from the list

[fabioh8010]

Thanks @parasharrajat , updated the list.

[hannojg]

https://github.com/Expensify/react-native/pull/33: Merged on our Fork. It seems that there is no Upstream PR. Could you confirm @hannojg?

This PR is for a fix on android to circumvent a ANR, we are working to get these changes into react native core. The tracking PRs can be found here:

• https://github.com/facebook/react-native/pull/38071

• https://github.com/facebook/react-native/pull/38073

Until they are merged I recommend making the verticalScrollbarPosition part of a patch for react native (its a relatively small and none invasive change I'd say)

[janicduplessis]

@fabioh8010 All fixes related to maintainVisibleContentPosition have been upstreamed and released in 0.72 except for https://github.com/Expensify/react-native/pull/16.

I will work on getting https://github.com/Expensify/react-native/pull/16 upstreamed but I don't think this is a blocker as we don't use this functionality currently.

[mountiny]

Ah I swear I remember someone mentioning that most of our stuff should be merged and release already so probably they did not have this spreadsheet. Thanks!

Which one of these do we need to block on if any. Can we use the patch for what @hannojg mentioned?

[fabioh8010]

@fabioh8010 All fixes related to maintainVisibleContentPosition have been upstreamed and released in 0.72 except for https://github.com/Expensify/react-native/pull/16.

I will work on getting https://github.com/Expensify/react-native/pull/16 upstreamed but I don't think this is a blocker as we don't use this functionality currently.

Thanks for the info @janicduplessis! Regarding to https://github.com/Expensify/react-native/pull/46, could you please confirm if this commit got merged on Upstream or is part of some fix? I was unable to find it in the source code and changelog.

---

Ah I swear I remember someone mentioning that most of our stuff should be merged and release already so probably they did not have this spreadsheet. Thanks!

@mountiny I got the spreadsheet from this message on Slack, I think we just didn't have enough visibility of the status of the PRs without doing a deeper investigation.

---

Which one of these do we need to block on if any. Can we use the patch for what @hannojg mentioned?

@hannojg From what I understood we would have to patch Android source files to apply the fix, right? If it's the case, it's not possible because the android sources are shipped pre-built during build process, so making a patch won't make difference.

@mountiny I updated the list with ❌ (possible blocker) and 🔨 (can be patched) labels. Currently, it seems to me that we have two possible blockers, https://github.com/Expensify/react-native/pull/37 and https://github.com/Expensify/react-native/pull/33. @aimane-chnaif @parasharrajat could you confirm if I'm missing something?

[fabioh8010]

As we're using RN directly, android bugs from our previously forked RN are gone:

https://github.com/Expensify/App/issues/17368 (fixed already but was workaround, all numberOfLines={100} occurrences can be safely removed) [HOLD][$1000] Web- Preferences - The recent mode description is truncated in Spanish preference #17509 (to fix, revert alternateTextMaxLines back to 0 from undefined) https://github.com/Expensify/App/issues/17441 (fixed)

@aimane-chnaif Just to clarify, I have to apply those 2 changes you suggested in this branch, right? all numberOfLines={100} occurrences can be safely removed and to fix, revert alternateTextMaxLines back to 0 from undefined.

[aimane-chnaif]

As we're using RN directly, android bugs from our previously forked RN are gone:

17368 (fixed already but was workaround, all numberOfLines={100} occurrences can be safely removed)

[HOLD][$1000] Web- Preferences - The recent mode description is truncated in Spanish preference #17509 (to fix, revert alternateTextMaxLines back to 0 from undefined)

17441 (fixed)

@aimane-chnaif Just to clarify, I have to apply those 2 changes you suggested in this branch, right? all numberOfLines={100} occurrences can be safely removed and to fix, revert alternateTextMaxLines back to 0 from undefined.

yes, it would be good but not a blocker.

[hannojg]

@fabioh8010 Yes, these are native changes, however its a change on the java files, not on the CPP files. The pre-shipped binaries are only for the native cpp libs afaik.

So that shouldn't be an issue 😊

[fabioh8010]

Hmm I think it also applies to Java files 🤔 , will test @hannojg and let you know, thanks.

[chrispader]

It's worth mentioning that it won't be possible to use patches to fix native issues in Android, as the Android sources are now shipped in pre-built artifacts. cc @chrispader

Yes, @fabioh8010 is right, we won't be able to include the maxNumberOfLines and numberOfLines feature @Szymon20000 implemented in the fork with just a patch. cc @mountiny

I'll check with @Szymon20000 how the progress in the upstream PR is going...

[fabioh8010]

Yes @chrispader @hannojg the prebuilts also include Java files, so any fixes to native Android side can't be patched.

It would be good if it was possible to come up with workarounds on JS side for now, until the pending PRs are merged and available in a future RN release, to avoid blocking this one.

[fabioh8010]

@aimane-chnaif I removed the occurrences of numberOfLines={100} and it's working as expected, but I wasn't able to reproduce the bug related to https://github.com/Expensify/App/issues/17509, could you please check?

[aimane-chnaif]

I removed the occurrences of numberOfLines={100} and it's working as expected

👍

I wasn't able to reproduce the bug related to https://github.com/Expensify/App/issues/17509, could you please check?

@fabioh8010 you mean on main or this branch?

[fabioh8010]

@aimane-chnaif this branch.

[aimane-chnaif]

@aimane-chnaif this branch.

Can you also test on main? https://github.com/Expensify/App/issues/17509 is web issue and this PR just upgraded RN version so web shouldn't be affected.

[janicduplessis]

Regarding to https://github.com/Expensify/react-native/pull/46, could you please confirm if this commit got merged on Upstream or is part of some fix? I was unable to find it in the source code and changelog.

@fabioh8010 It was merged as part of https://github.com/facebook/react-native/pull/35993

[fabioh8010]

@aimane-chnaif It turns out that PriorityModePage is using another component now, SelectionListRadio. This component list items don't use this alternateTextMaxLines prop, so we can safely remove alternateTextMaxLines from PriorityModePage.

[aimane-chnaif]

@fabioh8010 yes, I see that PR was merged yesterday and PriorityModePage is no longer using OptionRow. Safe to remove

[hannojg]

@fabioh8010 Kindly disagreeing here, the react-native core files are shipped as source with the npm react-native package (and it will stay this way).

You can verify yourself by checking e.g. node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/

[AndrewGable]

FYI - Excited for this one so I kicked off another AdHoc build

[janicduplessis]

🔴 / 🔨 https://github.com/Expensify/react-native/pull/16: Merged on our Fork. There is no Upstream PR yet, @janicduplessis will open one but it seems that is a functionality that is not used yet in the project.

Upstream PR opened https://github.com/facebook/react-native/pull/38245

[fabioh8010]

@fabioh8010 Kindly disagreeing here, the react-native core files are shipped as source with the npm react-native package (and it will stay this way).

You can verify yourself by checking e.g. node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/

@hannojg Yes, there are indeed Java files in that folder but modifying them won't make any difference when building the app (I just tested here as well), as stated in this guide we will be consuming precompiled artifacts when building for Android.

However, I also discovered that we can configure the project to build from source and by doing that it will be possible to apply changes to those files. I was able to apply your fix by configuring the project to build from source (the scroll indicator is on the correct side now):

https://github.com/Expensify/App/assets/20051562/b62cec7b-ba0c-43dc-b59b-1aad2db1a3a0

@roryabraham @mountiny WDYT about this? In this way we can patch the two fixes https://github.com/Expensify/react-native/pull/37 and https://github.com/Expensify/react-native/pull/33. The first disadvantage that I see is that the patch file will be gigantic, but it would allows us to move forward with this PR.

[roryabraham]

@fabioh8010 building from source sure is a bummer because of increased build times, but it's an acceptable cost since we seem to be using those upstream features.

The first disadvantage that I see is that the patch file will be gigantic

We're working w/ the maintainer of patch-package to alleviate this problem by allowing multiple patches. You can follow that progress here:

• https://github.com/Expensify/App/issues/20770
• https://github.com/ds300/patch-package/issues/43
• https://github.com/ds300/patch-package/pull/474

[roryabraham]

Actually, I just remembered that there is a canary release available for multiple patches published from this PR branch: https://github.com/ds300/patch-package/pull/474

So I'd suggest we start using it right away rather than creating a monolithic patch. Any feedback would be very helpful.

You can do npm install patch-package@canary to try out the --append flag. I think the workflow will be like so:

1. Make the changes from https://github.com/facebook/react-native/pull/35703, then run npx patch-package --append 'NumberOfLines'
2. Make the changes from https://github.com/facebook/react-native/pull/38073, then run npx patch-package --append 'VerticalScrollBarPosition'

[roryabraham]

Actually, just re-reviewing these PRs more carefully:

• https://github.com/facebook/react-native/pull/35703 – this adds numberOfLines and maximumNumberOfLines support for TextInput, TextArea, EditText components. However, I looked through all usages of numberOfLines in our repo, and they are all on standard Text components. So I think this may currently be unused.
• https://github.com/facebook/react-native/pull/38073 – I don't think we need this either, since we already have another workaround in place: https://github.com/Expensify/App/blob/b8aeec0bcf2cac06d51129f287f932ba0421ed4e/src/components/InvertedFlatList/index.android.js#L21

So it looks like neither of these are true blockers and we don't need to use the patches after all. Sorry, I just got excited about our new tool there... but I'm even more excited about not having to build Android from source.

[fabioh8010]

@roryabraham Just to clarify better the issues:

• https://github.com/facebook/react-native/pull/35703 We have some usages of maximumNumberOfLines in the Composer component for native platforms. Without this PR the composer will grow infinitely as reported by @aimane-chnaif.

https://github.com/Expensify/App/assets/20051562/d431b075-f5b6-47c5-bc4b-5cac9d4bfca3

• https://github.com/facebook/react-native/pull/38073 Without this PR the vertical scroll indicator will be on the wrong side on Android.

https://github.com/Expensify/App/assets/20051562/32f24266-2758-4b08-901e-fe27c362fa42

If we are OK with having this issues while they are not available in Upstream RN yet, we can safely merge this PR 👍