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
feat(android): add bridgeless support #4985
feat(android): add bridgeless support #4985
Conversation
@lukmccall thank you so much for sharing this code, I'm wondering if we should create a separate branch from Master and work on including the support for the new arch fully. what I see missing from the RN team release announcement for bridgeless mode:
@jan-kozinski @mateki0 this looks very promising to dive deeper into. |
My pull request is compatible with or without Bridgeless. Therefore, we can merge it gradually. It's your call. I am fine with both versions. |
Code itself looks fine for me and there is no regression. But I'm wondering if this is expected behaviour with enabled bridgeless and new architecture mode? I'm not sure if this is because we are still missing some newArch setup or it's something that should be fixed with this PR? I agree with adding support for iOS but we can do it in another PR and maybe this PR will be helpful in this case https://github.com/software-mansion/react-native-screens/pull/1990/files#top |
@mateki0, how did you test it? I wasn't able to reproduce the NPE. I built the example app using the It seems to be working fine. |
Ok so it's my bad because I was testing on 73.0 because post linked by Salah is saying that it was introduced in 73. I wasn't able to make 74 rc.1 working because of reanimated error. I even tried reanimated newest nightly release. Now I have checked it on 73.5 and there's no NPE error so it all looks fine. Your screen is from application with newArchDisabled, right? |
I see. My screen was taken in the project with the new arch enabled. That project is using the My PR only focuses on the newest RN version because the bridgeless mode is enabled by default with that version. However, that should probably work on 73, too, but I didn't test it. |
Ok I can confirm that it works fine on 74.rc.1 with new architecture, bridgeless mode and without reanimated it works fine. So I think it can be merged safely |
@lukmccall @mateki0 I was in touch with a core react-native developer regarding this, and he said that those changes are supposed to be done on top of the "migration guide for libraries for new arch" specifically the following needs to be followed: before enabling bridgeless mode. so I wouldn't start here and follow the recommended advise |
@salah-ghanim In the long, the library should be updated to support the new architecture fully. However, at present, I don't see any reason why we can't use the interop layer between the old and new architectures. It isn't a simple task to support Fabric in your project, and it will take some time. Therefore, why can't we use the interop layer now to unblock some projects from using the new architecture? Moreover, using the interop layer will make future migration easier because you can already compile your project with RN |
@salah-ghanim - @lukmccall and I are working with the React Native team at Meta on improving the state of the new architecture support in popular libraries by testing them and helping with fixes in core or in the library when needed. the best way to move forward on this in the short term is with this PR so it works well with the interop layer, and in the longer term it could be worthwhile to invest in an implementation that doesn't require the interop layer. |
Hey hey, Nicola here from the React Native team,
Agree to what @brentvatne and @lukmccall mentioned. The rationale behind the Interop Layer is to facilitate migrations and don't hard block the entire ecosystem on single library 👍 |
@salah-ghanim I agree we should merge this PR into master as a first step to the new architecture support. I'd really opt for gradual changes merged into staging frequently where possible as opposed to having a giant |
@brentvatne @cortinico @lukmccall thanks a lot for the support here all the extra help is appreciated :) I guess the information I received wasn't 100% accurate. I will merge this and work on updating the example (if not done yet) to latest RN |
# [1.11.0](v1.10.4...v1.11.0) (2024-03-10) ### Features * **android:** add bridgeless support ([#4985](#4985)) ([3ad0265](3ad0265))
🎉 This PR is included in version 1.11.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [1.11.0](react-native-maps/react-native-maps@v1.10.4...v1.11.0) (2024-03-10) ### Features * **android:** add bridgeless support ([#4985](react-native-maps/react-native-maps#4985)) ([3ad0265](react-native-maps/react-native-maps@3ad0265))
Does any other open PR do the same thing?
No
What issue is this PR fixing?
This PR add supports the bridgeless mode introduced in the new React Native 0.74 on Android.
How did you test this PR?
I have updated the RN version in the example app to RC and enabled the new architecture in the example app. Then, I tested the basic functionality with the bridgeless mode on both an Android emulator and a OnePlus 9 device. Additionally, I have also evaluated the same logic without the new architecture using the same testing method.