status-mobile icon indicating copy to clipboard operation
status-mobile copied to clipboard

research for enabling new architecture in react-native

Open siddarthkay opened this issue 2 years ago • 8 comments

Feature Issue

This issue will contain research for enabling new architecture in react-native.

  • [x] Implement a mechanism to use patch files -> https://github.com/status-im/status-mobile/pull/19451
  • [x] Upgrade react-native-camera-roll lib to 7.2.x & move code in fork to a patch. -> https://github.com/status-im/status-mobile/pull/19664
  • [x] Upgrade react-native-permissions lib to 4.x -> https://github.com/status-im/status-mobile/pull/19695
  • [ ] Upgrade react-native-gesture-handler lib to 2.12.x
  • [x] iOS still runs on JSC we need to enable Hermes there first. -> https://github.com/status-im/status-mobile/pull/19748
  • [x] For iOS we need to do prepend RCT_NEW_ARCH_ENABLED=1 flag along with pod install to enable new architecture. We run pod install during our build process here : nix/mobile/ios/shells/cocoapods.nix -> https://github.com/status-im/status-mobile/pull/19748
  • [ ] For Android we need to set newArchEnabled flag to true at android/gradle.properties

Libraries that support new architecture and WIP : https://github.com/reactwg/react-native-new-architecture/discussions/6#discussion-3897049

siddarthkay avatar Dec 11 '23 06:12 siddarthkay

When we enable new architecture for iOS the first thing that breaks is the react-native-permissions library :

[React-RCTFabric] Building library libReact-RCTFabric.a
⚠️  /Users/siddarthkumar/code/experiments/status-mobile/ios/Pods/Pods.xcodeproj: The iOS Simulator deployment target 'IPHONEOS_DEPLOYMENT_TARGET' is set to 11.0, but the range of supported deployment target versions is 12.0 to 17.0.99. (in target 'RNPermissions' from project 'Pods')
WriteAuxiliaryFile /Users/siddarthkumar/Library/Developer/Xcode/DerivedData/StatusIm-fxiitjbbybrcsqcxqartjfyvqxdo/Build/Intermediates.noindex/Pods.build/Debug-iphonesimulator/RNPermissions.build/RNPermissions-project-headers.hmap (in target 'RNPermissions' from project 'Pods')
    cd /Users/siddarthkumar/code/experiments/status-mobile/ios/Pods
[RNPermissions] Compiling RNPermissions-dummy.m
[RNPermissions] Compiling RNPermissionsModule.mm
❌ /Users/siddarthkumar/code/experiments/status-mobile/ios/Pods/Headers/Public/RCTTypeSafety/RCTTypeSafety/RCTConvertHelpers.h:46:53: 'value' is unavailable: introduced in iOS 12.0
  return vec.has_value() ? RCTConvertVecToArray(vec.value(), convertor) : nil;
                                                    ^
[RNPermissions] Compiling RNPermissionsHelper.m
[RNPermissions] Compiling RNPermissionsModule.mm
❌ /Users/siddarthkumar/code/experiments/status-mobile/ios/Pods/Headers/Public/RCTTypeSafety/RCTTypeSafety/RCTConvertHelpers.h:46:53: 'value' is unavailable: introduced in iOS 12.0
  return vec.has_value() ? RCTConvertVecToArray(vec.value(), convertor) : nil;
                                                    ^
[RNPermissions] Compiling RNPermissionsHelper.m

seems like https://github.com/zoontek/react-native-permissions/releases/tag/4.0.0 release has a fix for this.

siddarthkay avatar Dec 11 '23 06:12 siddarthkay

When we enable new architecture for Android the first thing that breaks is the react-native-gesture-handler library :

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/siddarthkumar/code/experiments/status-mobile/node_modules/react-native-gesture-handler/android/build.gradle' line: 197

* What went wrong:
A problem occurred evaluating project ':react-native-gesture-handler'.
> Project with path ':ReactAndroid' could not be found in project ':react-native-gesture-handler'.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 42s
error Failed to install the app.
make: *** [run-android] Error 1

seems like https://github.com/software-mansion/react-native-gesture-handler/releases/tag/2.12.0 has support for new architecture.

siddarthkay avatar Dec 11 '23 07:12 siddarthkay

update : react-native-gesture-handler error went away after upgrading that library to 2.12.0

The next thing to break is react-native-camera-roll with the following message

> Task :react-native-camera-kit:compileDebugKotlin
'compileDebugJavaWithJavac' task (current target is 11) and 'compileDebugKotlin' task (current target is 1.8) jvm target compatibility should be set to the same Java version.

> Task :react-native-camera-roll_camera-roll:compileDebugJavaWithJavac FAILED

and

Note: Recompile with -Xlint:deprecation for details.
/Users/siddarthkumar/code/experiments/status-mobile/node_modules/@react-native-camera-roll/camera-roll/android/src/main/java/com/reactnativecommunity/cameraroll/CameraRollModule.java:67: error: CameraRollModule is not abstract and does not override abstract method getPhotoThumbnail(String,ReadableMap,Promise) in NativeCameraRollModuleSpec
public class CameraRollModule extends NativeCameraRollModuleSpec {
       ^
Note: /Users/siddarthkumar/code/experiments/status-mobile/node_modules/@react-native-camera-roll/camera-roll/android/src/main/java/com/reactnativecommunity/cameraroll/CameraRollModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/siddarthkumar/code/experiments/status-mobile/node_modules/@react-native-camera-roll/camera-roll/android/src/main/java/com/reactnativecommunity/cameraroll/CameraRollPackage.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':react-native-camera-roll_camera-roll:compileDebugJavaWithJavac'.
> Compilation failed; see the compiler error output for details.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 1m 26s
error Failed to install the app.
info Run CLI with --verbose flag for more details.
make: *** [run-android] Error 1

siddarthkay avatar Dec 11 '23 08:12 siddarthkay

If I just remove the fork of react-native-camera-roll and use the library's latest version 7.2.0 I get the builds to work on android, but the onboarding screens crash within a few seconds with this message :

Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 9378 (.ethereum.debug), pid 9378 (.ethereum.debug)
Abort message: '/Users/siddarthkumar/code/experiments/status-mobile/node_modules/
react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp:539: 
function operator(): assertion failed (family.getSurfaceId() == surfaceId_)'

siddarthkay avatar Dec 12 '23 10:12 siddarthkay

Now that react-native upgrade is merged -> https://github.com/status-im/status-mobile/pull/18563

Resuming work on this and I think its better at this point to first fix how we apply patches to status-mobile.

Due to the limitations of patch phase we mostly relied on making forks of libraries and these forks are often outdated. If we move to a mechanism of applying patches we could then perform library upgrades with ease and it would be neater too.

We should also begin research into effort it would take to migrate native modules to JSI.

siddarthkay avatar Mar 29 '24 07:03 siddarthkay

@BalogunofAfrica suggested we use this npx package -> npx new-arch to keep track of what libraries support new architecture and below were the results.

This forms a good checklist for us

Scanned packages:

🔴 react-native-config: you have v1.5.0 – tested, may not work

❔ react-native-image-resizer: 1.2.3 – not tested

⬆️  @react-native-community/blur – update at least to 4.4.0 (currently 4.3.3)

⬆️  @react-native-async-storage/async-storage – update at least to 1.22.3 (currently 1.19.3)

❔ @react-native-camera-roll/camera-roll: 5.10.0 – not tested

🔴 @react-native-clipboard/clipboard: you have v1.13.2 – tested, may not work
     🔄 expo-clipboard - recommended alternative

❔ @react-native-community/audio-toolkit: 2.0.3 – not tested

🔴 @react-native-community/masked-view: you have v0.1.9 – tested, may not work
     🔄 @react-native-masked-view/masked-view - recommended alternative

⬆️  @react-native-community/netinfo – update at least to 11.3.1 (currently 4.7.0)

❔ @react-native-community/push-notification-ios: 1.4.1 – not tested

🔴 @react-native-community/slider: you have v3.0.0 – tested on RN0.74.0-nightly-20240123-cbd818dad, may not work

❔ react-native-background-timer: 2.2.0 – not tested

❔ react-native-biometrics: 3.0.1 – not tested

⬆️  react-native-blob-util – update at least to 0.19.6 (currently 0.13.18)

❔ react-native-camera-kit: 14.0.0-beta13 – not tested

❔ react-native-dialogs: 1.1.2 – not tested

🔴 react-native-fast-image: you have v8.5.11 – tested, may not work
     🔄 expo-image - recommended alternative
     😴 PR open / waiting for response from maintainer

🔴 react-native-fs: you have v2.16.6 – tested, may not work
     🔄 react-native-fs by @birdofpreyru or expo-file-system - recommended alternative

⬆️  react-native-gesture-handler – update at least to 2.16.0-rc.0 (currently 2.14.1)

❔ react-native-hole-view: 3.0.0-alpha4 – not tested

🔴 react-native-image-crop-picker: you have v0.40.0 – tested, may not work
     🔄 expo-image-picker - recommended alternative
     😴 PR open / contacted

⬆️  react-native-keychain – update at least to 8.2.0 (currently 8.1.2)

⬆️  react-native-linear-gradient – update at least to 2.8.3 (currently 2.8.0)

❔ react-native-lottie-splash-screen: 1.1.2 – not tested

❔ react-native-mail: 6.1.1 – not tested

❔ react-native-navigation: 7.38.3 – not tested

❔ react-native-orientation-locker: 1.5.0 – not tested

⬆️  react-native-permissions – update at least to 4.1.4 (currently 3.8.0)

🔴 react-native-reanimated: you have v3.6.1 – tested on RN0.74.0-nightly-20240123-cbd818dad, may not work
     ⏳ Ready, pending release

❔ react-native-shake: 3.4.0 – not tested

🟢 react-native-share: 10.0.2 – tested successfully on RN0.74.0-rc.3

❔ react-native-static-safe-area-insets: 2.2.0 – not tested

⬆️  react-native-svg – update at least to 15.2.0-rc.0 (currently 13.10.0)

❔ react-native-transparent-video: 0.1.0 – not tested

⬆️  react-native-webview – update at least to 13.8.4 (currently 13.6.3)

siddarthkay avatar Apr 05 '24 08:04 siddarthkay

iOS Works flawlessly but Android Fails with a runtime error AndroidBlurView is not Fabric compatible yet

Screenshot 2024-04-19 at 2 19 55 PM

siddarthkay avatar Apr 19 '24 12:04 siddarthkay

we're not done yet

siddarthkay avatar May 14 '24 10:05 siddarthkay

https://github.com/reactwg/react-native-new-architecture/discussions/189

We’re expecting to release the New Architecture as stable by the end of 2024.

Over the last few months we’ve fixed many New Architecture bugs and we believe the experience is now close to stable. Moreover, most of the https://github.com/reactwg/react-native-new-architecture/discussions/167 have been migrated to fully support the New Architecture.

siddarthkay avatar May 20 '24 03:05 siddarthkay

I tried to enable new architecture for Android on top of this PR -> https://github.com/status-im/status-mobile/pull/20308 which removes blur library usage on Android. However I am met with next incompatible library react-native-fast-image

Screenshot 2024-06-04 at 5 19 26 PM

siddarthkay avatar Jun 04 '24 11:06 siddarthkay

it does seem like there is an issue which has no response for new architecture support -> https://github.com/DylanVann/react-native-fast-image/issues/925 There is also a PR from react-native team to add new architecture support here -> https://github.com/DylanVann/react-native-fast-image/pull/1032#issuecomment-2072784676

I'll try upgrading to use the patch but it does seem better to try alternative libraries at this point.

siddarthkay avatar Jun 04 '24 11:06 siddarthkay

it does seem like there is an issue which has no response for new architecture support -> DylanVann/react-native-fast-image#925 There is also a PR from react-native team to add new architecture support here -> DylanVann/react-native-fast-image#1032 (comment)

I'll try upgrading to use the patch but it does seem better to try alternative libraries at this point.

I think looking for an alternative would be better. Author doesn't seem to be interested and not handing over to willing maintainers https://x.com/dylan_h_vann/status/1600956747144896526

BalogunofAfrica avatar Jun 04 '24 12:06 BalogunofAfrica

I think looking for an alternative would be better

Yes I tried the patch but it didn't work and I also looked at alternatives that support new architecture but couldn't find any. I've created this issue to remove react-native-fast-image -> https://github.com/status-im/status-mobile/issues/20327

siddarthkay avatar Jun 05 '24 03:06 siddarthkay

Another thing that breaks on Android is react-native-linear-gradient for this library support is available in 3.0.0-alpha.1 -> https://github.com/react-native-linear-gradient/react-native-linear-gradient/issues/622#issuecomment-1673881541

Screenshot 2024-06-05 at 9 04 10 AM

siddarthkay avatar Jun 05 '24 03:06 siddarthkay

upgrading react-native-linear-gradient to 3.0.0-alpha.1 fixes the issue. Next incompatible warning on Android is this. Screenshot 2024-06-05 at 10 32 10 AM

siddarthkay avatar Jun 05 '24 05:06 siddarthkay

seems like react-native-camera-kit does not plan to support new architecture any time soon -> https://github.com/teslamotors/react-native-camera-kit/issues/537 This could be a problem and we may have to find alternatives to it.

siddarthkay avatar Jun 05 '24 05:06 siddarthkay

we can drop react-native-fast-image no problem, https://github.com/mrousavy/react-native-vision-camera this is way better than react-native-camera-kit

flexsurfer avatar Jun 05 '24 08:06 flexsurfer

hm https://github.com/mrousavy/react-native-vision-camera/issues/2614

flexsurfer avatar Jun 05 '24 08:06 flexsurfer

i'm wondering why there is no backward compatibility, there should be, both versions of libs should work just fine, no?

flexsurfer avatar Jun 05 '24 08:06 flexsurfer

both versions of libs should work just fine, no?

yes it would make sense for Android to behave like iOS. And I think I found the issue being in Android Native code. I am verifying it locally.

siddarthkay avatar Jun 05 '24 13:06 siddarthkay

we can drop react-native-fast-image no problem

@flexsurfer, could you expand on why you think we can drop it? My thinking is, why did devs in the past added it to status-mobile and now it's suddenly fine to remove it? I would assume it solves a real problem, but perhaps you are suggesting the standard Image will deliver the same experience to users in our app?

ilmotta avatar Jun 05 '24 16:06 ilmotta

@ilmotta sure, it was added before the media server when we had lots of base64 images, and with react-native-fast-image they worked better and faster, also react-native-fast-image has aggressive caching, now with the media server we don't need it, because URL always different because of the port rotation, and we use react-native-fast-image in some places, just as a leftover, but it's fine to remove it, also I remember @OmarBasem had issues with react-native-fast-image and he replaced it with react image

flexsurfer avatar Jun 06 '24 09:06 flexsurfer

@ilmotta sure, it was added before the media server when we had lots of base64 images, and with react-native-fast-image they worked better and faster, also react-native-fast-image has aggressive caching, now with the media server we don't need it, because URL always different because of the port rotation, and we use react-native-fast-image in some places, just as a leftover, but it's fine to remove it, also I remember @OmarBasem had issues with react-native-fast-image and he replaced it with react image

I see, if the request is almost always a cache miss, then fast-image is doing nothing but overhead. @flexsurfer, I'm hoping switching to Image won't cause flikering when the port is the same, because, for example, if the user stays in a chat and don't switch apps, the port will be the same, in which case FastImage would be better than Image.

ilmotta avatar Jun 06 '24 12:06 ilmotta

@ilmotta sure, it was added before the media server when we had lots of base64 images, and with react-native-fast-image they worked better and faster, also react-native-fast-image has aggressive caching, now with the media server we don't need it, because URL always different because of the port rotation, and we use react-native-fast-image in some places, just as a leftover, but it's fine to remove it, also I remember @OmarBasem had issues with react-native-fast-image and he replaced it with react image

I see, if the request is almost always a cache miss, then fast-image is doing nothing but overhead. @flexsurfer, I'm hoping switching to Image won't cause flikering when the port is the same, because, for example, if the user stays in a chat and don't switch apps, the port will be the same, in which case FastImage would be better than Image.

it's hard to say if it would be better or not, but cache should be disabled for FastImage so I would say someone who will be working on that should make some investigations first

flexsurfer avatar Jun 06 '24 14:06 flexsurfer

it's hard to say if it would be better or not, but cache should be disabled for FastImage so I would say someone who will be working on that should make some investigations first

Agreed! 👍🏼

ilmotta avatar Jun 06 '24 14:06 ilmotta

also I remember @OmarBasem had issues with react-native-fast-image and he replaced it with react image

@flexsurfer I think you are referring to that performance issue originally raised by @clauxx

if the user stays in a chat and don't switch apps, the port will be the same, in which case FastImage would be better than Image.

@ilmotta if we are handling caching externally then I believe FastImage and Image would be the same (for caching), but I am not sure how our media server works, if it works (or potentially can work) like I was suggesting here then it shouldn't matter

OmarBasem avatar Jun 06 '24 16:06 OmarBasem

also I remember @OmarBasem had issues with react-native-fast-image and he replaced it with react image

@flexsurfer I think you are referring to that performance issue originally raised by @clauxx

if the user stays in a chat and don't switch apps, the port will be the same, in which case FastImage would be better than Image.

@ilmotta if we are handling caching externally then I believe FastImage and Image would be the same (for caching), but I am not sure how our media server works, if it works (or potentially can work) like I was suggesting here then it shouldn't matter

Yeah @OmarBasem I'm not entirely sure, but I think our media server is not generating the cache headers that FastImage can take advantage of. But I never investigated this part of FastImage to know how the lack of cache headers can affect it, but surely in its documentation this is mentioned. It does look as you guys are saying that it will be fine to replace it with Image 👍🏼

ilmotta avatar Jun 06 '24 17:06 ilmotta

Hey, we at Software Mansion have been working on improving support for the new architecture for quite a while now. You can check how we helped Expensify App do it here: https://blog.swmansion.com/sunrising-new-architecture-in-the-new-expensify-app-729d237a02f5. Would you like our help with migrating your app?

WoLewicki avatar Aug 02 '24 11:08 WoLewicki

Thanks @flexsurfer for recommending https://github.com/reactwg/react-native-new-architecture/discussions/135 so for now we could tell react-native to not use fabric for the following components that were found to be incompatible on Android :

  • 'BVLinearGradient',
  • 'CKCameraManager',
  • 'FastImageView',

siddarthkay avatar Aug 02 '24 12:08 siddarthkay

@WoLewicki : Thanks for the offer, I really appreciate your offer to help with this! Should we need any more assistance I would reach out to you ❤️

siddarthkay avatar Aug 02 '24 12:08 siddarthkay