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

Support new architecture #100

Open
nandorojo opened this issue Apr 3, 2024 · 8 comments
Open

Support new architecture #100

nandorojo opened this issue Apr 3, 2024 · 8 comments

Comments

@nandorojo
Copy link
Collaborator

nandorojo commented Apr 3, 2024

Lifting #84 into a new issue.

Problem

New Architecture isn't working with this library.

Context

React Native 0.74 is coming with bridgeless mode and improvements for the new architecture. It appears that Expo is finally adopting it and they have a plan to test libraries for it.

How to test it

Here brent explains how to test New Architecture support with an Expo Module (it's easy): https://x.com/notbrent/status/1774931733194088465?s=20

TLDR: install expo-build-properties into the app you're testing, and add it to your app.json, set newArchEnabled: true.

Show code
{
  "expo": {
    "name": "try-zeego",
    "slug": "try-zeego",
    "icon": "./assets/icon.png",
    "splash": {
      "image": "./assets/splash.png",
      "resizeMode": "contain",
      "backgroundColor": "#ffffff"
    },
    "ios": {
      "supportsTablet": true,
      "bundleIdentifier": "com.example.with-new-arch"
    },
    "android": {
      "package": "com.example.withnewarch"
    },
    "plugins": [
      [
        "expo-build-properties",
        {
          "ios": {
            "newArchEnabled": true
          },
          "android": {
            "newArchEnabled": true
          }
        }
      ]
    ]
  }
}

Here is a reproduction showing that this library does not currently work with the new architecture.

There's no crash, but the menu doesn't open.

Solution

Fabric deprecates findNodeHandle(), and it looks like react-native-ios-context-menu uses that function.

I think the solution would be to remove findNodeHandle in favor of the component's ref.

I'm not sure if other changes would be needed, but I'd assume that would fix it.

This also removes the bridge, so I think self.bridge would not be an option to use either.

@fobos531
Copy link

fobos531 commented Apr 4, 2024

This also removes the bridge, so I think self.bridge would not be an option to use either.

About this, it seems that bridgeless mode still exposes a "bridge" in some way, probably to ensure maximum backwards compatibility

CleanShot 2024-04-04 at 10 39 51@2x

So hopefully it might not be a big hindrance

@nandorojo
Copy link
Collaborator Author

nandorojo commented Apr 10, 2024

Turns out self.bridge isn't in the source anyway so I think it's a moot point. We will have figure out findNodeHandle though

@fobos531
Copy link

fobos531 commented Apr 10, 2024

Turns out self.bridge isn't in the source anyway so I think it's a moot point. We will have figure out findNodeHandle though

self.bridge is not directly utilized by react-native-ios-context-menu, but it looks like it is heavily utilized by react-native-ios-utilities, which react-native-ios-context-menu depends on. Either way, I agree, both these concerns should be addressed to ensure 0.74 + bridgeless compatibility.

cc @dominicstop

@nandorojo
Copy link
Collaborator Author

Oh, got it, good to know. Well at least we know that will work then.

@dominicstop
Copy link
Owner

dominicstop commented Apr 11, 2024

Hi, thanks for tagging me, the issue got buried in my inbox, so I apologize for the very late reply...

Yes, i agree that this is important, however It looks like I’ll need to do some refactors + run some experiments to support the new architecture — the one thing i’m worried about the most is that the workarounds/hacks i’ve used in the past won’t work anymore w/ fabric/JSI...

regarding findNodeHandle, a view's associated react tag can be sent from JS to Native via an event — but you can also retrieve it via onLayout (or reading a private property in the native component's ref).

As for the bridge usage, i'm not sure yet if there's a corresponding replacement for all the API's i'm using...

I’m currently working on making a component that wraps UITableView, but ran into some trouble due to the delay/async communication between native and JS.


Below is a quick demo:

2024-04-11-21-44-43.mp4

The demo shows the ff. things:

  • Using regular RN components for the table view’s cell content.
  • drag and drop, and cell re-ordering (via UITableViewDiffableDataSource + UIDragDelegate).
  • Cell reuse (i.e. the state of the RN cell content is preserved across rows since it’s being reused, hence the counter stays the same across rows, but the content changes).
  • Dynamic cell height (i.e self-sizing cells).
  • Mapping JS data to the table view data source.
  • etc..

i'm not sure yet, but i think the new architecture + expo API's will fix this...

@fobos531
Copy link

fobos531 commented Apr 11, 2024

As for the bridge usage, i'm not sure yet if there's a corresponding replacement for all the API's i'm using...

@dominicstop I haven't checked in full detail, but it seems like you're not using any APIs that the RN team has explicitly specified that the RCTBridgeProxy does not support. So, it may work "out of the box" as long as you switch to using this RCTBridgeProxy instead of the RCTBridge

@fobos531
Copy link

Hello @dominicstop

As a heads up, RN 0.74 was officially released yesterday, which means it is likely that there will be an increase in the number of users actively experimenting with the new architecture, which will probably bring more attention to this issue.

@dominicstop
Copy link
Owner

dominicstop commented Apr 25, 2024

hello, thank you for the gentle warning/reminder; i made 2 new branches so i can start working on this (i.e. react-native-ios-utilities/wip-new-arch, and react-native-ios-context-menu/wip-new-arch).


Unfortunately, i won't be able to support the "aux. previews" for now (i'm hoping to fix support for it in future releases); but i'll be able to make sure all the other stuff work (my best estimate is around 2-3 days).

i understand your frustration, and i apologize. In summary, i'm currently in between places (and i lost my current job that allows me to work on OSS full-time). As such, for the foreseeable future, i'm going to be supporting this library in my free time only (as a hobby), since i'm going to go back to my non-tech job again. But for now, i still have a couple of days left of paid time to work on supporting the new architecture.


TLDR: I'm working on it, and i'm hoping to finish it before the end of the month (i'll update this thread once i'm able to make a pre-release version for testing).


thank you for understanding ✨


Updates


  • Update:2024-04-26-13:06 PST - It turns out a lot of weird stuff is happening w/ fabric (please see the "Log + Status" section for the detailed report). I'm still looking at what i can do to fix support for fabric, but it's currently not looking good haha (it might take more than 3 days asdjshkfsdl).

  • Update:2024-04-30-15:36 PST - Made some small progress in making a wrapper to use swift views in fabric. Please see log for more details.

  • Update:2024-05-23-03:16 PST - The swift "compatibility layer" for fabric and paper has been mostly completed. E.g. most of the common API's needed for exposing native views to RN has been implemented (i.e. RNIBaseView + RNIContentView).

# New Architecture Support - Log + Status

  • 2024-04-26-04:08 PST - react-native-ios-context-menu (Commits)
    • Desc: Updated example to enable new architecture.

  • 2024-04-26-05:05 PST - react-native-ios-context-menu
    • Problem: RNIContextMenuView not receiving touch events.
    • Setting pointerEvents to none for RNIContextMenuView does nothing.

  • 2024-04-26-08:47 PST - react-native-ios-context-menu
    • Attachments: 🖼️ Screenshot 2024-04-26-9:10:46-AM.png, 📄Logs-2024-04-26-09-24-00.txt.
    • Observation: RNIContextMenuView has only one parent view called ExpoFabricView.
      • This parent view is the root view, there are no other siblings to RNIContextMenuView, it doesn't have a gesture recognizer attached to it, and it doesn't have a reference to a window.
      • The frame and bounds of RNIContextMenuView is zero, however i haven't tried checking the layoutMetrics yet.
      • It's as if RNIContextMenuView is isolated and not part of the view hierarchy?
      • Maybe: the reason why the RNIContextMenuView is not receiving touch events is because of the responder chain system not being respected, i.e. the gesture recognizers in RNIContextMenuView does not have a view to forward the touch events.
      • Maybe: UIContextMenuInteraction attaches a bunch of gesture recognizers to RNIContextMenuView, and once a touch events occurs, there's no way for it to "bubble up" to the current window.

  • 2024-04-26-09:37 PST - react-native-ios-context-menu
    • Attachments: 🖼️ Screenshot-2024-04-26-9:45:38-AM.png, 🖼️ Screenshot-2024-04-26-9:43:42-AM.
    • Observation: It seems that the view hierarchy is mangled (or obfuscated) for exported views somehow, and simply traversing the view hierarchy is not sufficient. It looks like we will have to use the UIManager to properly traverse the views.
      • Note: It seems that even Xcode's "Debug View Hierarchy" is having some trouble too. Although it throws an error, you can still inspect the view hierarchy (although some of the debugging features don't work sadly).

  • 2024-04-26-09:57 PST - react-native-ios-context-menu
    • Attachments: 🖼️ Screenshot-2024-04-26-9:56:19 AM.png
    • Observation: RNIContextMenuView is being wrapped by something called: ViewManagerAdapter_RNIContextMenuView.
      • Unfortunately, Xcode's "Debug View Hierarchy" is not able to inspect any of the views (it cannot print out any debug info regarding the views).
      • We have access to the raw memory address of the instance though, so maybe we can manually inspect ViewManagerAdapter_RNIContextMenuView via lldb.

    • 2024-04-26-10:48 PST - react-native-ios-context-menu (Commits)
      • Attachments: 🖼️ Screenshot-2024-04-26-10:48:03-AM.png, 🖼️ Screenshot-2024-04-26-10:57:4-AM.png.
      • Observation: Just to be completely sure, i traversed and printed out all the parent responder chain.
        • Same as previous tests, it appears that the RNIContextMenuView is completely isolated.
        • There is a way to break out of containment though (i.e. via swizzling), but we will use that as a last resort (for now, let's just keep looking).

    • 2024-04-26-11:11 PST - react-native-ios-context-menu
      • Attachments: 🖼️ Screenshot-2024-04-26-11:11:23-AM.png, 🖼️ Screenshot 2024-04-26-11:27:47-AM.
      • Observation: Unfortunately, even if i use the raw pointer for the RCTViewComponentView instance, and then using LLDB expressions to poke it, i am unable to get any information out of it because LLDB just gives up (same tbh).
        • Potential Reason: All of the stuff for the new architecture is written in objc++, and LLDB might not be able to inspect the types/instances due to this fact.
        • Using LLDB is a dead-end, it looks like we will have to look at the actual (generated) code itself to continue.
        • However, using Xcode's memory graph, we can at least see inside RCTViewComponentView, and it's relationship with other objects.

      • 2024-04-26-13:01:55 PST - react-native-ios-context-menu
        • Attachments: 🖼️ Screenshot-2024-04-26-1:03-PM.png
        • For this session, we are going to look at: ContextMenuViewExample01. Here are the memory addresses of the instances we are focusing on, just so we can keep track of things (please see the the attachment above for a visual guide):
          • ((RNIContextMenuView *)0x14663de00).
          • ((ViewManagerAdapter_RNIContextMenuView *)0x145d2ff90).
          • ((RCTViewComponentView *)0x1501e78a0).

        • The first thing we can observe from the view hierarchy, is that the "child views" of ContextMenuViewExample01 are not inside of RNIContextMenuView.
        • If we check the recursive subviews for ((RNIContextMenuView *)0x14663de00), we get: recursive subview count: 0 (please see attachment below for more details).
        • Now let's take a look at ((ViewManagerAdapter_RNIContextMenuView *)0x145d2ff90) in the memory graph.
          • Attachment: 🖼️ Screenshot-2024-04-26-2:30:51-PM.png
          • Some extra info, the class name for this instance is actually: ExpoFabricView.
          • It's a wrapper that holds``((RNIContextMenuView *)0x14663de00)`.
          • Weirdly enough, the memory graph inspector says that ((ViewManagerAdapter_RNIContextMenuView *)0x145d2ff90) holds a reference to the current window, but if you try to access it inside ((RNIContextMenuView *)0x14663de00) it just return nil.

        • Now let's take a look at: ((RCTViewComponentView *)0x1501e78a0) in the memory graph.
          • This is the view that contains content displayed in ContextMenuViewExample01.
            • Attachment: 🖼️ Screenshot 2024-04-26-2:40:45-PM
            • Pre-fabric, RNIContextMenuView.subviews contains the views rendered in ContextMenuViewExample01 .
            • However on fabric, this is no longer the case; a subview in ViewManagerAdapter_RNIContextMenuView (i.e. ExpoFabricView) holds the views that are shown in ContextMenuViewExample01 (please see attachment above for more details).

          • Via the memory graph, we can see that ((RCTViewComponentView *)0x1501e78a0) is being referenced by an object instance called RCTComponentViewRegistry.
            • Attachment: 🖼️ 2024-04-26-2:46:10-PM.png
            • It seems that "tags" (e.g. reactTag, nodeHandle, etc) still exist in fabric.
            • But instead of being just an NSNumber, it's a C++ primitive type called: facebook::react::Tag (it looks like it's just a wrapper for a JSI::Value though, so it's still just a integer value).

        • 2024-04-27-06:17 PST - Research
          • It looks like our exported view RNIContextMenuView, is being wrapped by ExpoFabricView, and ExpoFabricView inherits from ExpoFabricViewObjC.
            • On the new architecture, ExpoFabricViewObjC inherits from RCTViewComponentView, otherwise it inherits from RCTView.
            • "Exported" views in the arch. have to be UIView<RCTComponentViewProtocol>.

          • In the old architecture, the way we "receive" views from native is via getting them in insertReactSubviews.
            • In the new architecture, we have a lifecycle method called mountChildComponentView and unmountChildComponentView.
            • It looks like ExpoFabricView being a descendant of RCTViewComponentView, is able to access those methods.

          • In the new architecture, we have to register our class in RCTComponentViewRegistry.
            • It looks like expo does this in our behalf, by dynamically creating a ExpoFabricView subclass that wraps our exported view (code snippet link).
            • This is why the superview of RNIContextMenuView is a type called: ViewManagerAdapter_RNIContextMenuView.
            • Via swizzling, the dynamically created class receives: __injectedAppContext, and __injectedModuleName.

  • 2024-04-28-20:15 PST - react-native-ios-utilities (Commits)
    • Attachments: 🖼️ Screenshot-2024-04-28-8:17:24-PM, 🖼️ Screenshot-2024-04-28-8:17:30-PM
    • Desc: Nuked repo, and started over from scratch via create-react-native-library. The screenshot shows a basic fabric view, and shows creating TestDummyClass instance that was declared in swift to test out swift + objc-c interop.

  • 2024-04-30-15:38 PST - react-native-ios-utilities (Commits)
  • 2024-05-01-00:05 PST - react-native-ios-utilities (Commits)
  • 2024-05-01-13:00 PST - Log
    • Attachments: 🖼️ Screenshot-2024-05-01-12:54:40-PM.png
    • The docs says that Object type is supported as a prop for the js spec passed to codegenNativeComponent, but when running codegen, it throws an error Error: Unknown prop type for "someObjectProp": "Object" at getTypeAnnotation.
    • As a temp. workaround, maybe we'll just stringify the object so they can be passed as strings, and parse it to create an NSDictionary?

  • 2024-05-02-18:37 PST - react-native-ios-utilities (Commits)
    • Attachments: 📼 Screen-Recording-2024-05-02-7:01:45-PM.mov
    • Desc: Impl. the ff. RNIViewLifecycleEventsNotifiable lifecycle events: notifyOnFinalizeUpdates, notifyOnPrepareForReuse, notifyOnMountChildComponentView, notifyOnUnmountChildComponentView.

  • 2024-05-23-03:09 PST - react-native-ios-utilities (Commits)
    • Desc: Implemented common swift API between fabric and paper (e.g. props, setSize, events, view commands, etc).

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

No branches or pull requests

3 participants