Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

research for enabling new architecture in react-native #18138

Open
6 of 7 tasks
siddarthkay opened this issue Dec 11, 2023 · 31 comments · Fixed by #19748 · May be fixed by #20950
Open
6 of 7 tasks

research for enabling new architecture in react-native #18138

siddarthkay opened this issue Dec 11, 2023 · 31 comments · Fixed by #19748 · May be fixed by #20950
Assignees
Milestone

Comments

@siddarthkay
Copy link
Contributor

siddarthkay commented Dec 11, 2023

Feature Issue

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

Libraries that support new architecture and WIP : reactwg/react-native-new-architecture#6 (comment)

@siddarthkay
Copy link
Contributor Author

siddarthkay commented Dec 11, 2023

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
Copy link
Contributor Author

siddarthkay commented Dec 11, 2023

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
Copy link
Contributor Author

siddarthkay commented Dec 11, 2023

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 added a commit that referenced this issue Dec 11, 2023
This PR does many things :
- Upgrade `react-native ` to `0.72.5`
- Upgrade `react-native-reanimated` to  `3.5.4`
- Upgrade `react-native-navigation` to `7.37.0`
- `ndkVersion` has been bumped to `25.2.9519653`
- `cmakeVersion` has been bumped to `3.22.1`
- `kotlinVersion` has been bumped to `1.7.22`
- `AGP` has been bumped to `7.4.2`
- `Gradle` has been upgraded to `8.0.1`
- Android `CompileSDK` and `TargetSDK` have been bumped to 33
- `@react-native-async-storage/async-storage` has been upgraded to `1.19.3`
- `@walletconnect/client` has been nuked
- some of the old `react-native-reanimated` code has been nuked
- `react-native-keychain` fork has been replaced with `8.1.2`

- On Android we are currently relying on `Hermes` Engine.
- On iOS we are currently relying on JSC
- We are not enabling new architecture for now (I have plans for that in the future) ref: #18138

IOS only PR : #16721
Android only PR : #17062

- `make run-metro` now has a target of `android` which was `clojure` earlier, this will increase the time it takes to start metro terminal but this is needed otherwise you will get a nasty error while developing for android locally.
siddarthkay added a commit that referenced this issue Dec 11, 2023
This commit does many things :
- Upgrade `react-native ` to `0.72.5`
- Upgrade `react-native-reanimated` to  `3.5.4`
- Upgrade `react-native-navigation` to `7.37.0`
- `ndkVersion` has been bumped to `25.2.9519653`
- `cmakeVersion` has been bumped to `3.22.1`
- `kotlinVersion` has been bumped to `1.7.22`
- `AGP` has been bumped to `7.4.2`
- `Gradle` has been upgraded to `8.0.1`
- Android `CompileSDK` and `TargetSDK` have been bumped to 33
- `@react-native-async-storage/async-storage` has been upgraded to `1.19.3`
- `@walletconnect/client` has been nuked
- some of the old `react-native-reanimated` code has been nuked
- `react-native-keychain` fork has been replaced with `8.1.2`

- On Android we are currently relying on `Hermes` Engine.
- On iOS we are currently relying on JSC
- We are not enabling new architecture for now (I have plans for that in the future) ref: #18138

IOS only PR : #16721
Android only PR : #17062

- `make run-metro` now has a target of `android` which was `clojure` earlier, this will increase the time it takes to start metro terminal but this is needed otherwise you will get a nasty error while developing for android locally.
siddarthkay added a commit that referenced this issue Dec 11, 2023
This commit does many things :
- Upgrade `react-native ` to `0.72.5`
- Upgrade `react-native-reanimated` to  `3.5.4`
- Upgrade `react-native-navigation` to `7.37.0`
- `ndkVersion` has been bumped to `25.2.9519653`
- `cmakeVersion` has been bumped to `3.22.1`
- `kotlinVersion` has been bumped to `1.7.22`
- `AGP` has been bumped to `7.4.2`
- `Gradle` has been upgraded to `8.0.1`
- Android `CompileSDK` and `TargetSDK` have been bumped to 33
- `@react-native-async-storage/async-storage` has been upgraded to `1.19.3`
- `@walletconnect/client` has been nuked
- some of the old `react-native-reanimated` code has been nuked
- `react-native-keychain` fork has been replaced with `8.1.2`

- On Android we are currently relying on `Hermes` Engine.
- On iOS we are currently relying on `JSC`
- We are not enabling new architecture for now (I have plans for that in the future) ref: #18138

IOS only PR : #16721
Android only PR : #17062

- `make run-metro` now has a target of `android` which was `clojure` earlier, this will increase the time it takes to start metro terminal but this is needed otherwise you will get a nasty error while developing for android locally.
@siddarthkay
Copy link
Contributor Author

siddarthkay commented Dec 12, 2023

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
Copy link
Contributor Author

Now that react-native upgrade is merged -> #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
Copy link
Contributor Author

siddarthkay commented Apr 5, 2024

@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 added a commit that referenced this issue Apr 17, 2024
This commit swaps the fork of @react-native-camera-roll/camera-roll with a patch and upgrades this library to the latest version.

needed for : #18138

Verify if camera album related features still work.

- iOS

status: ready
siddarthkay added a commit that referenced this issue Apr 17, 2024
This commit swaps the fork of @react-native-camera-roll/camera-roll with a patch and upgrades this library to the latest version.

needed for : #18138

Verify if camera album related features still work.

- iOS

status: ready
siddarthkay added a commit that referenced this issue Apr 17, 2024
This commit swaps the fork of @react-native-camera-roll/camera-roll with a patch and upgrades this library to the latest version.

needed for : #18138

Verify if camera album related features still work.

- iOS

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit upgrades `react-native-permissions` library to latest version 4.1.5

needed for : #18138

## Review & Test notes

Please test permissions related flows on onboarding, selecting images, notifications and any other areas you can think of.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit upgrades `react-native-permissions` library to latest version 4.1.5

needed for : #18138

## Review & Test notes

Please test permissions related flows on onboarding, selecting images, notifications and any other areas you can think of.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit swaps the fork of react-native-mail with a patch.

needed for : #18138

## Review & Test notes

Verify if mailing logs still works.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit swaps the fork of @react-native-community/blur with a patch and upgrades the library to latest version of 4.4.0

needed for : #18138

## Review & Test notes

Verify if blur stuff still works.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit swaps the fork of react-native-mail with a patch.

needed for : #18138

## Review & Test notes

Verify if mailing logs still works.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit swaps the fork of react-native-mail with a patch.

needed for : #18138

## Review & Test notes

Verify if mailing logs still works.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit swaps the fork of @react-native-community/blur with a patch and upgrades the library to latest version of 4.4.0

needed for : #18138

## Review & Test notes

Verify if blur stuff still works.

## Platforms
- iOS
- Android

status: ready
siddarthkay added a commit that referenced this issue Apr 18, 2024
## Summary

This commit upgrades `react-native-permissions` library to latest version 4.1.5

needed for : #18138

## Review & Test notes

Please test permissions related flows on onboarding, selecting images, notifications and any other areas you can think of.

## Platforms
- iOS
- Android

status: ready
@siddarthkay
Copy link
Contributor Author

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.

@BalogunofAfrica
Copy link
Contributor

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

@siddarthkay
Copy link
Contributor Author

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 -> #20327

@siddarthkay
Copy link
Contributor Author

siddarthkay commented Jun 5, 2024

Another thing that breaks on Android is react-native-linear-gradient
for this library support is available in 3.0.0-alpha.1 -> react-native-linear-gradient/react-native-linear-gradient#622 (comment)

Screenshot 2024-06-05 at 9 04 10 AM

@siddarthkay siddarthkay moved this to In Progress in Status Desktop/Mobile Board Jun 5, 2024
@siddarthkay
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@flexsurfer
Copy link
Member

flexsurfer commented Jun 5, 2024

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
Copy link
Member

@flexsurfer
Copy link
Member

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

@siddarthkay
Copy link
Contributor Author

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.

@ilmotta
Copy link
Contributor

ilmotta commented Jun 5, 2024

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?

@flexsurfer
Copy link
Member

@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

@ilmotta
Copy link
Contributor

ilmotta commented Jun 6, 2024

@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.

@flexsurfer
Copy link
Member

@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

@ilmotta
Copy link
Contributor

ilmotta commented Jun 6, 2024

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! 👍🏼

@OmarBasem
Copy link
Contributor

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

@ilmotta
Copy link
Contributor

ilmotta commented Jun 6, 2024

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 👍🏼

@WoLewicki
Copy link

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?

@siddarthkay
Copy link
Contributor Author

Thanks @flexsurfer for recommending reactwg/react-native-new-architecture#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
Copy link
Contributor Author

@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 ❤️

@ilmotta ilmotta added this to the 2.32.0 Beta milestone Oct 22, 2024
@churik
Copy link
Member

churik commented Nov 26, 2024

I don't think that we have a capacity for it in 2.32, given the desired release cut date.
Deferring the issue, please be free to move it bak if you think there is a room for it @ilmotta

@churik churik modified the milestones: 2.32.0, 2.33.0 Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment