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

React-native 0.68.* is not compatible with RNN right now - exposing createReactActivityDelegate - and support for new architecture (Fabric) #7466

Closed
SudoPlz opened this issue Feb 22, 2022 · 30 comments · Fixed by #7744

Comments

@SudoPlz
Copy link
Collaborator

SudoPlz commented Feb 22, 2022

🐛 Bug Report

Hey there,

I'm opening that issue to keep track of react-native 0.68 support. Right now it seems that react-native navigation doesn't work with 0.68 and the new architecture.

Specifically the part where we're supposed to inject ReactRootViews seems to be the pain point, since RNN doesn't export any way to do that currently.

https://github.com/react-native-community/rn-diff-purge/blob/release/0.68.0-rc.1/RnDiffApp/android/app/src/main/java/com/rndiffapp/MainActivity.java#L23

cc'ing @yogevbd on this to hear some thoughts.

@yogevbd
Copy link
Collaborator

yogevbd commented Feb 23, 2022

Hey @SudoPlz,
We currently don't have an Android developer working on RNN, we will have to somehow support it soon though.

@danilobuerger
Copy link
Collaborator

@SudoPlz as far as I can tell this only affects 0.68 when trying to enable the new architecture, right? 0.68 should work when keeping the old way, right?

@SudoPlz
Copy link
Collaborator Author

SudoPlz commented Feb 23, 2022

@danilobuerger not really sure to be honest, this requires testing.

@danilobuerger
Copy link
Collaborator

Alright, I will take a look at that once 0.68 rc2 is out.

@danilobuerger
Copy link
Collaborator

So, I investigated this using the old architecture for now.

iOS

There is a breaking change in the app template, that we should communicate in the docs:

diff --git a/ios/feastr/Classes/AppDelegate.m b/ios/feastr/Classes/AppDelegate.m
index d29d69933..8b67f1d11 100644
--- a/ios/feastr/Classes/AppDelegate.m
+++ b/ios/feastr/Classes/AppDelegate.m
@@ -111,7 +111,7 @@ - (void)applicationWillEnterForeground:(UIApplication *)application {
 
 - (NSURL *)sourceURLForBridge:(RCTBridge *)bridge {
 #if DEBUG
-    return [[RCTBundleURLProvider sharedSettings] jsBundleURLForBundleRoot:@"index" fallbackResource:nil];
+    return [[RCTBundleURLProvider sharedSettings] jsBundleURLForBundleRoot:@"index"];
 #else
     return [[NSBundle mainBundle] URLForResource:@"main" withExtension:@"jsbundle"];
 #endif

Android

We need a new product flavor. The RootView interface added two more methods that we need to implement in ModalContentLayout:

  /**
   * Called when a child starts a native gesture (e.g. a scroll in a ScrollView). Should be called
   * from the child's onTouchIntercepted implementation.
   */
  void onChildStartedNativeGesture(View childView, MotionEvent ev);

  /**
   * Called when a child ends a native gesture. Should be called from the child's onTouchIntercepted
   * implementation.
   */
  void onChildEndedNativeGesture(View childView, MotionEvent ev);
diff --git a/node_modules/react-native-navigation/lib/android/app/src/main/java/com/reactnativenavigation/react/modal/ModalContentLayout.kt b/node_modules/react-native-navigation/lib/android/app/src/main/java/com/reactnativenavigation/react/modal/ModalContentLayout.kt
index da7d5b1..ea8516f 100644
--- a/node_modules/react-native-navigation/lib/android/app/src/main/java/com/reactnativenavigation/react/modal/ModalContentLayout.kt
+++ b/node_modules/react-native-navigation/lib/android/app/src/main/java/com/reactnativenavigation/react/modal/ModalContentLayout.kt
@@ -49,9 +49,15 @@ class ModalContentLayout(context: Context?) : ReactViewGroup(context), RootView{
             updateFirstChildView()
         }
     }
+    override fun onChildStartedNativeGesture(child: View, androidEvent: MotionEvent?) {
+        mJSTouchDispatcher.onChildStartedNativeGesture(androidEvent, this.getEventDispatcher())
+    }
     override fun onChildStartedNativeGesture(androidEvent: MotionEvent?) {
         mJSTouchDispatcher.onChildStartedNativeGesture(androidEvent, this.getEventDispatcher())
     }
+    override fun onChildEndedNativeGesture(child: View, androidEvent: MotionEvent?) {
+        mJSTouchDispatcher.onChildEndedNativeGesture(androidEvent, this.getEventDispatcher())
+    }
     override fun requestDisallowInterceptTouchEvent(disallowIntercept: Boolean) {}
     private fun getEventDispatcher(): EventDispatcher? {
         val reactContext: ReactContext = this.getReactContext()

@danilobuerger
Copy link
Collaborator

@yogevbd I think we should leave this open until we also support the new architecture?

@yogevbd yogevbd reopened this Feb 28, 2022
@yogevbd
Copy link
Collaborator

yogevbd commented Feb 28, 2022

Your'e right.

@SudoPlz SudoPlz changed the title React-native 0.68.* is not compatible with RNN right now - exposing createReactActivityDelegate React-native 0.68.* is not compatible with RNN right now - exposing createReactActivityDelegate - and support for new architecture Feb 28, 2022
@SudoPlz SudoPlz changed the title React-native 0.68.* is not compatible with RNN right now - exposing createReactActivityDelegate - and support for new architecture React-native 0.68.* is not compatible with RNN right now - exposing createReactActivityDelegate - and support for new architecture (Fabric) Feb 28, 2022
@danilobuerger
Copy link
Collaborator

I started looking at iOS + new arch (rnn 7.26.0, rn 0.68 rc2 with pending patched for rc3):

  • We have to manually enable rnn to use the farbic views (which currently is a good thing, but we should do that automatically) by adding the following pod:
pod 'ReactNativeNavigation/Fabric', :path => "../node_modules/react-native-navigation"

I got this to compile and work. So far only with waitToRender turned off and the views seem misaligned.

@YazeedAsaad
Copy link

When can we expect a release for 0.68.0 support since the stable released yesterday ?

@danilobuerger
Copy link
Collaborator

@YazeedAsaad rnn does work with rn 0.68 in its default configuration (old architecture).

@YazeedAsaad
Copy link

it does indeed, i meant the new Architecture if that’s planned

@danilobuerger
Copy link
Collaborator

I am not aware that any one is working on it. But feel free to submit a PR if changes are required to make it work with the new architecture.

@oblador
Copy link
Contributor

oblador commented Apr 1, 2022

I would perfer if it was possible to opt into Fabric on a per screen basis as it would enable incremental adoption.

@LarsJK

This comment was marked as off-topic.

@danilobuerger

This comment was marked as off-topic.

@LarsJK

This comment was marked as off-topic.

@doublex

This comment was marked as off-topic.

@danilobuerger

This comment was marked as off-topic.

@bfricka
Copy link

bfricka commented Apr 25, 2022

@doublex this issue is about getting rnn to work with the new architecture. So your issue is not related. Feel free to create a new one about the linker, or even better create a PR with a fix.

I get where you're coming from, but both the docs and rnn-link do not work, so the happy path is broken, and this is probably why people are having issues. For those having issues on a freshly initialized 0.68 project, you can run npx rnn-link, then remove the body from MainActivity (or just comment it out):

public class MainActivity extends NavigationActivity {}

Without this, the build fails because NavigationActivity doesn't extend ReactActivity and doesn't have a createReactActivityDelegate method.

@enahum
Copy link

enahum commented Jun 23, 2022

Does anyone know if someone is already working in this? Not that I'm offering, just wanted to see if there is any progress

@kentdev92
Copy link

Does anyone know if someone is already working in this? Not that I'm offering, just wanted to see if there is any progress

Please try to add this to settings.gradle:
include ':react-native-navigation'
project(':react-native-navigation').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-navigation/lib/android/app/')

@SudoPlz
Copy link
Collaborator Author

SudoPlz commented Oct 4, 2022

@guyca I noticed you worked on #7518.

  • Does this add support for RN 0.68?
  • Should I close this ticket? Is it official, can we use the new architecture now?
  • If not, are there extra steps the RNN team should take to support that, and is there a place where you track the progress? Right now all I'm aware of is the current issue.

Thanks for all your hard work.

@oferRounds
Copy link

oferRounds commented Nov 24, 2022

@yogevbd @guyca – following up on @SudoPlz question: is it possible to use RNN with React Native 0.68+?

@guyca
Copy link
Collaborator

guyca commented Nov 24, 2022

@oferRounds Yes, we use it with RN 0.68.4.

@oferRounds
Copy link

Thanks @guyca! so that means RNN use the new Arch?

@SudoPlz
Copy link
Collaborator Author

SudoPlz commented Dec 20, 2022

@guyca I noticed you worked on #7518.

  • Does this add support for RN 0.68?
  • Should I close this ticket? Is it official, can we use the new architecture now?
  • If not, are there extra steps the RNN team should take to support that, and is there a place where you track the progress? Right now all I'm aware of is the current issue.

Thanks for all your hard work.

@guyca any news with that?

Thank you 🙏

@guyca
Copy link
Collaborator

guyca commented Dec 25, 2022

@SudoPlz We're using RNN and RN 68 at Wix. AFAIK there are no issues with that. Regarding the new architecture - I think we plan on researching the possibility of switching to it sometime in Q1/Q2 2023. @yogevbd correct me if I'm wrong.

@ivan-khr85
Copy link

Any updates?

@erennyuksell
Copy link

Any updates?

@yogevbd yogevbd mentioned this issue Aug 2, 2023
4 tasks
yogevbd added a commit that referenced this issue Aug 3, 2023
- [x] Upgrade react-native to 0.72
- [x] Add new architechture & fabric support
- [x] Fix auto linking
- [x] Test backward compatibility

Closes #7753
Closes #7547
Closes #7466
Closes #7742
@SudoPlz
Copy link
Collaborator Author

SudoPlz commented Aug 11, 2023

This has been implemented and rolled out on 7.36.0.

I managed to run our app using Fabric and RNN 7.36.0 but I did discover one issue: #7770.
I'll chat about it with @yogevbd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.