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.14k stars 2.63k forks source link

Upgrade to RN 0.74 #37374

Open roryabraham opened 4 months ago

roryabraham commented 4 months ago

slack context: https://expensify.slack.com/archives/C02NK2DQWUX/p1709024519896479

Problem

There are some new arch problems that should be fixed in RN 0.74. So far that's the only thing I'm aware of in 0.74 that we'll need, but it takes a long time to upgrade. It's not necessarily a blocker but unless we upgrade we'll have to use patches.

Solution

Let's get the ball rolling on the RN 0.74 upgrade. If there are issues in the RC we should report them to Meta.

MrRefactor commented 4 months ago

Hey, I would like to work on this issue!

roryabraham commented 4 months ago

@MrRefactor I'd strongly recommend you to try https://github.com/infinitered/flame. There's a talk from Jamon it's creator here. I hope it will help streamline the upgrade process for you.

MrRefactor commented 4 months ago

@roryabraham Sure! I will get gpt-4 and try upgrading with that tool!

roryabraham commented 4 months ago

wrote some best practices on RN upgrades earlier today:

  1. Look at the release notes to understand what changed.
  2. Look at any patches that we've got, find the upstream PR associated with the patch by looking at the PR that introduced the patch: there should be a link to an upstream PR for the patch in the PR description. Evaluate if that patch is still needed or can be removed. If it's still needed, coordinate with the author of the patch to figure out how it needs to be applied to the new version. Have them update their upstream PR if needed. Also consider alternatives if Meta doesn't seem to be actively reviewing the PR. Open a conversation in slack if you're not sure.
  3. If there are any deprecations, investigate what's needed to remove our usage of deprecated code. It doesn't necessarily need to be a blocker, but we want to try to remove uses of deprecated APIs ASAP.
  4. Use flame, an AI-powered CLI made specifically for upgrading React Native. It uses react-native-upgrade-helper under the hood and leverages GPT-4 to apply the diff more intelligently. Very cool tool, you can see a demo here.
  5. Use 2 C+ testers instead of just 1
  6. Create an AdHoc build and have Applause run a full regression suite
  7. Create a GitHub Project for the upgrade so that if any issues are identified, they can be added to the project and tracked effectively, rather than just being comments in the PR. That way we can track what issues need to be resolved and re-tested on staging.
MrRefactor commented 4 months ago

Update:

  1. Tried upgrading with flame-ai but as its still in experimental stage its not really working as expected with 0.74, opened an issue with one of the problems connected with that.
  2. Manually applied changes from upgrade-helper - I will create draft PR and will be adding any progress there
  3. I will go through all of the patches and list them to make sure if upgrade of rn version is affecting those
MrRefactor commented 4 months ago

Update:

  1. Working on rc-2 as it was released couple days ago
  2. Working on resolving issues with dependencies, trying to apply temporary workaround to get app running on android & ios
  3. Resolving issues with react-native-quick-sqlite connected to wrong compilation of Folly on iOS
  4. Resolving issues with react-native-live-markdown on android
  5. Checking compatibility in android dependencies connected to gradle versioning
Screenshot 2024-03-08 at 20 43 33 Screenshot 2024-03-08 at 19 58 57

CC: @roryabraham @mountiny

MrRefactor commented 4 months ago

Update:

Created an issue in react-native-quick-sqlite as the same error is visible while building with fresh rn app. In meantime working on a fix for that problem.

roryabraham commented 4 months ago

Created an https://github.com/margelo/react-native-quick-sqlite/issues/37 in react-native-quick-sqlite as the same error is visible while building with fresh rn app. In meantime working on a fix for that problem.

Posted in the Margelo room

MrRefactor commented 4 months ago

Update:

Created an issue in @expensify/react-native-live-markdown as the same error is visible while building with fresh rn app for android.

MrRefactor commented 4 months ago

Update:

  1. Switching to RC.3 as it was released
  2. Working on patches for @expensify/react-native-live-markdown and react-native-quick-sqlite
tomekzaw commented 4 months ago

@MrRefactor The fix for @expensify/react-native-live-markdown is pretty simple, it's just a matter of changing a single import in MarkdownUtils.java:

-import com.facebook.react.views.text.CustomLineHeightSpan;
+import com.facebook.react.views.text.internal.span.CustomLineHeightSpan;
MrRefactor commented 4 months ago

@tomekzaw Thanks Tomek!

MrRefactor commented 4 months ago

Update: Implemented workarounds for:

  1. @expensify/react-native-live-markdown
  2. react-native-quick-sqlite

Working on new issues connected with:

Android:

  1. lottie-react-native
  2. expo , expo-modules-core,

iOS

  1. expo-modules-core

I will also create issue & pr in @react-native-async-storage/async-storage as its showing warnings with mismatched versions of ksp version

MrRefactor commented 4 months ago

Also there was a question from @cead22 if we could upgrade rn to 0.73.5 in meantime as its fixing this issue

CC: @mountiny @roryabraham

roryabraham commented 4 months ago

commented in slack, but I think we should press on with the 0.74 upgrade

roryabraham commented 4 months ago

@MrRefactor, let's start doing updates in slack to get more visibility, then just link to the slack updates here in GH.

How can we help push this along? What are you doing to parallelize the work here? Can we get more people involved to split up the work?

roryabraham commented 4 months ago

@MrRefactor looking forward to seeing an update from you on this project.

MrRefactor commented 3 months ago

Update on upgrade work: https://expensify.slack.com/archives/C01GTK53T8Q/p1711111014179099

CC: @roryabraham @mountiny

shubham1206agra commented 3 months ago

@roryabraham, I'm happy to help if a C+ is needed here.

roryabraham commented 3 months ago

latest discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1711111014179099

MrRefactor commented 3 months ago

Update:

I will also post an update on slack - sorry for late update, needed to get back to speed after Easter.

roryabraham commented 3 months ago

@MrRefactor do you have an estimate when this PR will be ready for review?

MrRefactor commented 3 months ago

@MrRefactor do you have an estimate when this PR will be ready for review?

Aiming at end of this week!

ikevin127 commented 3 months ago

We got another Fabric related issue for which I proposed a patch:

Discussing it on Slack 🧵, this RN v0.74 commit came up:

Which might fix our issue and the patch won't be required once we're on v0.74. We're currently debating whether to add the proposed patch or HOLD until we're upgraded.

roryabraham commented 3 months ago

I'm wondering if this issue may also have been fixed in RN 0.74

MrRefactor commented 3 months ago

Update - 19.04:

Draft PR link -> link

After merging new arch to main, I needed to revert most of the changes as those have been resolved in new arch pr. Also as we are adding a bridgeless for new architecture it may require more effort as we estimated at the beginning.

There is one breaking issue connected to NotificationService target on iOS - /App/ios/NotificationServiceExtension/NotificationService.swift:8:8 Compiling for iOS 13.0, but module 'AirshipServiceExtension' has a minimum deployment target of iOS 14.0 I have messaged @arosiclair as he created that target couple of months ago. When we switch to higher deployment target its crashing with implementation issues: Screenshot 2024-04-17 at 17 05 50 (1)

Expo update: today expo released its first beta version of SDK51 for react-native 0.74.0 - link After updating all expo libraries it seems all building issues connected to expo are resolved on android side, for iOS need to resolve NotificationService issue first to make sure its working fine.

I have removed/updated react native patches introduced in new arch PR, as some of them has been resolved in rn 0.74.0

As for android - after updating/patching libs current build blocker is connected to react-native-clipboard and its alignment to rn0.74.0 but as I can see @WoLewicki has already stared aligning that in this PR

Also in this thread @WoLewicki and @j-piasecki asked if they could join rn-upgrade efforts as there is many more libraries to be aligned with new changes in RN core

I'm wondering if https://github.com/Expensify/App/issues/40108#issuecomment-2062563600 may also have been fixed in RN 0.74

Im checking it right now and will give an update on that in next post on monday.

CC: @mallenexpensify @roryabraham @mountiny @WoLewicki @j-piasecki @arosiclair

shubham1206agra commented 3 months ago

I'm wondering if this issue may also have been fixed in RN 0.74

@MrRefactor If the Android build is working, can you check this issue, as it is critical and deploy blocking?

MrRefactor commented 3 months ago

If Android build is working, can you check this issue too as it is critical and deploy blocking.

Android build is not working as commented here:

As for android - after updating/patching libs current build blocker is connected to react-native-clipboard and its alignment to rn0.74.0 but as I can see @WoLewicki has already stared aligning that in this https://github.com/react-native-clipboard/clipboard/pull/226

WoLewicki commented 3 months ago

Commenting

j-piasecki commented 3 months ago

Hi!

MrRefactor commented 2 months ago

Update 22.04:

Draft PR link -> https://github.com/Expensify/App/pull/40548

Hi! Yesterday & today I managed to resolve issues with Airship - now it has min deployment version of 14.0 as marked in this commit.

I also created/updated patches for failing libraries while building on iOS & Android:

Build issues:

iOS -> issue related to nrmapbox/map: Screenshot 2024-04-22 at 17 15 07 Screenshot 2024-04-22 at 17 15 11

Android -> issue related to react-native-vision-camera - will open an issue in vision camera repo:

e: /App/node_modules/react-native-vision-camera/android/src/main/java/com/mrousavy/camera/frameprocessors/VisionCameraProxy.kt:31:79 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.
e: App/node_modules/react-native-vision-camera/android/src/main/java/com/mrousavy/camera/frameprocessors/VisionCameraProxy.kt:36:19 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.
e: /App/node_modules/react-native-vision-camera/android/src/main/java/com/mrousavy/camera/frameprocessors/VisionCameraProxy.kt:36:47 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.
e: /App/node_modules/react-native-vision-camera/android/src/main/java/com/mrousavy/camera/frameprocessors/VisionCameraProxy.kt:75:73 This API is provided only for React Native frameworks and not intended for general users. This API can change between minor versions in alignment with React Native frameworks and won't be considered a breaking change.

CC: @mallenexpensify @roryabraham @mountiny @WoLewicki

shubham1206agra commented 2 months ago

@mrousavy Can you help with the above comment?

mrousavy commented 2 months ago

@shubham1206agra VisionCamera is a bit tricky to bring to new arch. I have asked the Meta team about some implementation specifics regarding the frameProcessor approach I currently use here: https://github.com/reactwg/react-native-new-architecture/discussions/167#discussioncomment-8938596

But so far I don't have a clear approach and I think this is still blocked. I am in contact with @philIip from meta and he's kind enough to help me find a solution to this using CodeGen soon.

mrousavy commented 2 months ago

Theoretically you could use the interop layer they built, and rip out all the Frame Processor stuff in VisionCamera Android - then it'll surely build.

But a real solution is to get away from the private APIs I currently use to inject the Frame Processor runtime into JSI, which I hopefully plan to solve with CodeGen/Fabric JSI View Managers.

WoLewicki commented 2 months ago

We don't use Frame Processors in the App, or am I missing something? Also, looking at those lines: https://github.com/mrousavy/react-native-vision-camera/blob/b4413c5f9e40a26dc8588b3b52584b7aa6e56e49/package/android/build.gradle#L81-L87, since we don't use react-native-worklets-core, Frame Processors should not even be added to the project. Or did something change recently @mrousavy ?

mrousavy commented 2 months ago

Hey - yup I know, Frame Processors are not enabled here anyways - but the VisionCameraProxy.kt file will still be compiled because Java doesn't support preprocessor flags, so I can't really compile that code out (unless I create another file that stubs out the implementations and switches the source files at build.gradle with srcSets, but that's a bit complex right now).

So these lines here in VisionCameraProxy.kt will still be compiled, and if React Native removes them it will fail to compile - even though we are not calling VisionCameraProxy in Expensify (because Frame Processors are not enabled).

So a workaround for now would be to rip out all the Frame Processors related stuff as a temporary patch, or to compile all that stuff out in build.gradle using srcSets - either of those is only a temporary workaround.

The real solution will be VisionCamera migrating over to new-arch w/ CodeGen, Fabric and TurboModules - but I am still a bit blocked on that since I don't know how to add a JSI-only property to the <Camera> view. I need to run C++ JSI code before that prop (frameProcessor) gets passed to native Java/Swift, kinda like an RCT_ENUM_CONVERTER but for JSI/C++.

mrousavy commented 2 months ago

I can get VisionCamera migrated to new arch / bridgeless for you next week or the week after that (currently on vacation in Italy 🇮🇹), is that cool with you guys? cc @roryabraham

mrousavy commented 2 months ago

Do we use react-native-mmkv? That's already migrated, only a minor fix then I can merge that https://github.com/mrousavy/react-native-mmkv/pull/656

MrRefactor commented 2 months ago

Do we use react-native-mmkv? That's already migrated, only a minor fix then I can merge that mrousavy/react-native-mmkv#656

We are not using react-native-mmkv

I can get VisionCamera migrated to new arch / bridgeless for you next week or the week after that (currently on vacation in Italy 🇮🇹), is that cool with you guys? cc @roryabraham

In meanwhile I will try to patch workaround for that issue

mountiny commented 2 months ago

I can get VisionCamera migrated to new arch / bridgeless for you next week or the week after that (currently on vacation in Italy 🇮🇹), is that cool with you guys

To understand, is this a blocker for the 0.74 PR? @MrRefactor

MrRefactor commented 2 months ago

I can get VisionCamera migrated to new arch / bridgeless for you next week or the week after that (currently on vacation in Italy 🇮🇹), is that cool with you guys

To understand, is this a blocker for the 0.74 PR? @MrRefactor

Yes it is, Im working on the workaround connected with excluding Frame Processors described by @mrousavy to allow us to build android.

MrRefactor commented 2 months ago

Update 23.04:

Main Draft PR link -> https://github.com/Expensify/App/pull/40548 Reanimated Draft PR link -> https://github.com/Expensify/App/pull/40775

Hi! Today I've been working on resolving issues reported yesterday with rnmapbox/maps and react-native-vision-camera - I wasn't able to fix them yet but work is ongoing.

I have also created a Draft PR for react-native-reanimated version upgrade according to @roryabraham comment here. It will be hard to extract every library outside of main RN upgrade PR as those rely on changes in RN 0.74.0 and may cause blockers with RN 0.73.X but will try to extract as many as possible.

Added commit to main PR changing version of RN from RC to official release -> link

I will mark react-native-reanimated PR open as soon as we make sure that it's working as expected.

CC: @mallenexpensify @roryabraham @mountiny @WoLewicki

MrRefactor commented 2 months ago

Update 24.04:

Main Draft PR link -> https://github.com/Expensify/App/pull/40548 Reanimated Draft PR link -> https://github.com/Expensify/App/pull/40775

Hi! Today I kept working on issues reported with android and iOS builds.

I have created an issue in rnmapbox/maps and posted questions about failing iOS builds here -> https://github.com/rnmapbox/maps/issues/3462 Also reproduced that issue on fresh react native 0.74.0 repo.

I have also created a patch for react-native-vision-camera removing all code related to frame processors - still testing that patch if its working properly. It seems like I have issues with codegen, so need to doublecheck it. Also locked version of react-native-vision-camera to make sure all patches introduced with new arch PR are still properly applied.

Also tomorrow @kbieganowski will start helping me with splitting this PR into smaller pieces to merge them to main before rest of the changes as discussed here -> https://github.com/Expensify/App/pull/40548#pullrequestreview-2015296489

CC: @mallenexpensify @roryabraham @mountiny

MrRefactor commented 2 months ago

Update 25.04:

Main Draft PR link -> https://github.com/Expensify/App/pull/40548 Reanimated Draft PR link -> https://github.com/Expensify/App/pull/40775

Hi! Today I managed to make some progress with android builds. iOS issue is still not resolved as I'm trying to implement support for 0.74 to rnmapbox/maps.

Updated libraries:

As going forward, I encountered an error related to new turbo modules linking introduced in 0.74.0. Error details: 

 /App/node_modules/react-native/ReactAndroid/cmake-utils/default-app-setup/OnLoad.cpp:74:10: error: use of undeclared identifier 'rncli_cxxModuleProvider'; did you mean 'rncli_ModuleProvider'?
    return rncli_cxxModuleProvider(name, jsInvoker);
           ^~~~~~~~~~~~~~~~~~~~~~~
           rncli_ModuleProvider
  /App/android/app/build/generated/rncli/src/main/jni/rncli.h:19:30: note: 'rncli_ModuleProvider' declared here
  std::shared_ptr<TurboModule> rncli_ModuleProvider(const std::string moduleName, const JavaTurboModule::InitParams &params);
                               ^
/App/node_modules/react-native/ReactAndroid/cmake-utils/default-app-setup/OnLoad.cpp:74:40: error: no viable conversion from 'const std::shared_ptr<CallInvoker>' to 'const JavaTurboModule::InitParams'
    return rncli_cxxModuleProvider(name, jsInvoker);
                                         ^~~~~~~~~
 /App/node_modules/react-native/ReactAndroid/build/prefab-headers/react_nativemodule_core/ReactCommon/JavaTurboModule.h:28:10: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'const std::shared_ptr<CallInvoker>' to 'const InitParams &' for 1st argument
    struct InitParams {
           ^
  /App/node_modules/react-native/ReactAndroid/build/prefab-headers/react_nativemodule_core/ReactCommon/JavaTurboModule.h:28:10: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'const std::shared_ptr<CallInvoker>' to 'InitParams &&' for 1st argument
/App/android/app/build/generated/rncli/src/main/jni/rncli.h:19:116: note: passing argument to parameter 'params' here
  std::shared_ptr<TurboModule> rncli_ModuleProvider(const std::string moduleName, const JavaTurboModule::InitParams &params);

I couldnt reproduce that on bare 0.74.0 react-native repo and I keep looking for a solution to that. One of the paths I want to check is usage of react-native-config whether its impacting that issue at any point.

CC: @mallenexpensify @roryabraham @mountiny @mrousavy @WoLewicki

MrRefactor commented 2 months ago

Update 26.04:

Main Draft PR link -> https://github.com/Expensify/App/pull/40548 Reanimated Draft PR link -> https://github.com/Expensify/App/pull/40775

Hi! I'm still resolving issues from previous PR - trying to fix new turbo modules linking errors as well as align rnmapbox/map to work with rn 0.74. I have also started working on 0.74.0 support for react-native-pager-view as its breaking android builds, and needs to be aligned as well.

CC: @mallenexpensify @roryabraham @mountiny @WoLewicki

roryabraham commented 2 months ago

@MrRefactor I just merged a PR that upgrades react-native-quick-sqlite with RN 0.74 compatibility

MrRefactor commented 2 months ago

Update 29.04:

Main Draft PR link -> https://github.com/Expensify/App/pull/40548 Reanimated Draft PR link -> https://github.com/Expensify/App/pull/40775

Hi! I'm still resolving android build issues connected to new linking of the packages.

Updated libraries:

Updated patches:

Added patches:

I have also patched react-native-pager-view for android, it seems to be working fine, need to create patch for iOS as well.

NOTE: I'm OOO starting Wednesday, will be back on Monday!

CC: @mallenexpensify @roryabraham @mountiny

MrRefactor commented 2 months ago

Update 30.04:

Main Draft PR link -> https://github.com/Expensify/App/pull/40548 Reanimated Draft PR link -> https://github.com/Expensify/App/pull/40775

Hi! I managed to make some progress with android builds - resolved codegen issues with react-native-pager-view, react-native-airship and react-native-geolocation - still need to fix react-native-webview codegen issues.

Screenshot 2024-04-30 at 20 11 51

I couldnt reproduce the issue on fresh rn 0.74 app.

NOTE: I'm OOO until the end of the week.

CC: @mallenexpensify @roryabraham @mountiny

trjExpensify commented 2 months ago

@roryabraham I'm putting this in the Summer release, but if there's a particular project that it's necessary for -- let me know. As I understand it, we didn't need it for QBO.

roryabraham commented 2 months ago

Ok, sounds good. Hopefully we'll finish this up soon after @MrRefactor is back from OOO