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

feat(android): add bridgeless support #4985

Conversation

lukmccall
Copy link
Contributor

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.

@salah-ghanim
Copy link
Collaborator

salah-ghanim commented Feb 25, 2024

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

  1. ViewConfig static typing
  2. iOS support

@jan-kozinski @mateki0 this looks very promising to dive deeper into.

link: reactwg/react-native-new-architecture#154

@lukmccall
Copy link
Contributor Author

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.

@mateki0
Copy link
Collaborator

mateki0 commented Feb 28, 2024

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?

Screenshot 2024-02-28 at 18 19 54

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
And the same goes with ViewConfig static typing, we need it but I would suggest to do it in another PR because it's just an improvement to what we have here.

@lukmccall
Copy link
Contributor Author

@mateki0, how did you test it? I wasn't able to reproduce the NPE. I built the example app using the rc.1 version:
image

It seems to be working fine.

@mateki0
Copy link
Collaborator

mateki0 commented Feb 29, 2024

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?

@lukmccall
Copy link
Contributor Author

I see. My screen was taken in the project with the new arch enabled. That project is using the 0.74.rc.1 version. I took your example project, bumped the RN version, removed reanimated, and applied minor changes to build the project with the newest RN version.

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.

@mateki0
Copy link
Collaborator

mateki0 commented Mar 3, 2024

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

@salah-ghanim
Copy link
Collaborator

@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:
https://reactnative.dev/docs/new-architecture-library-intro

before enabling bridgeless mode.

so I wouldn't start here and follow the recommended advise

@lukmccall
Copy link
Contributor Author

lukmccall commented Mar 4, 2024

@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 0.74 and bridgeless enabled.

@brentvatne
Copy link
Contributor

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

@cortinico
Copy link

Hey hey, Nicola here from the React Native team,

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.

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 👍

@jan-kozinski
Copy link
Collaborator

@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 beta PR that adds everything at once.

@salah-ghanim
Copy link
Collaborator

@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

@salah-ghanim salah-ghanim merged commit 3ad0265 into react-native-maps:master Mar 10, 2024
4 checks passed
react-native-maps-bot pushed a commit that referenced this pull request Mar 10, 2024
# [1.11.0](v1.10.4...v1.11.0) (2024-03-10)

### Features

* **android:** add bridgeless support ([#4985](#4985)) ([3ad0265](3ad0265))
@react-native-maps-bot
Copy link
Collaborator

🎉 This PR is included in version 1.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Super-CodeKing added a commit to Super-CodeKing/react_native_map that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants