dominicstop / react-native-ios-context-menu

A react-native component to use context menu's (UIMenu) on iOS 13/14+
MIT License
562 stars 26 forks source link

Support new architecture #100

Open nandorojo opened 7 months ago

nandorojo commented 7 months ago

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 ```json { "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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

dominicstop commented 6 months ago

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:

https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/c9d76b04-581a-4991-ba74-a9bddde4afe9

The demo shows the ff. things:

fobos531 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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



# New Architecture Support - Log + Status

* `2024-04-26-04:08 PST` - `react-native-ios-context-menu` ([Commits](https://github.com/dominicstop/react-native-ios-context-menu/compare/7243284...fee24d0)) * **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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/c36d3a46-940a-4ace-b794-fceb14e886cf), 📄[Logs-2024-04-26-09-24-00.txt](https://github.com/dominicstop/react-native-ios-context-menu/files/15124134/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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/d4f80f3f-916b-42bc-abb8-5f693f01ab1e), 🖼️ [Screenshot-2024-04-26-9:43:42-AM](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/aacf79e2-c684-4d82-8db2-b6e418ffdfa5). * **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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/761dc804-e100-4059-9c23-1aab3dd0304c") * **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](https://github.com/dominicstop/react-native-ios-context-menu/compare/fee24d0...fc69757)) * **Attachments**: 🖼️ [Screenshot-2024-04-26-10:48:03-AM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/93cc1d1e-1a06-47ad-89cb-4713d55a9aaf), 🖼️ [Screenshot-2024-04-26-10:57:4-AM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/de293d13-e89e-480a-bf85-78c258b8918c). * **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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/b4ac12fb-11c7-4c89-aeac-8b8903a5db40), 🖼️ [Screenshot 2024-04-26-11:27:47-AM](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/b2d95e2e-d7df-4194-9fe2-a769de27fb20). * **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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/ff32bda7-2651-4365-9274-7097aef933b8) * 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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/ff32bda7-2651-4365-9274-7097aef933b8) 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). * **Attachments**: 🖼️ [Screenshot-2024-04-26-1:44:20-PM](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/5cc8262d-021c-4e9a-90a8-1377b2aeb594)

* Now let's take a look at `((ViewManagerAdapter_RNIContextMenuView *)0x145d2ff90)` in the memory graph. * **Attachment**: 🖼️ [Screenshot-2024-04-26-2:30:51-PM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/5d09fbf9-87cf-4fba-8762-8c3a652c2bbd) * 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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/6280edba-38b0-46e0-aaa1-6fcff0be3c5c) * 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](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/6280edba-38b0-46e0-aaa1-6fcff0be3c5c) for more details).

* Via the memory graph, we can see that `((RCTViewComponentView *)0x1501e78a0)` is being referenced by an object instance called [`RCTComponentViewRegistry`](https://github.com/facebook/react-native/blob/main/packages/react-native/React/Fabric/Mounting/RCTComponentViewRegistry.h). * **Attachment**: 🖼️ [2024-04-26-2:46:10-PM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/87275dac-0c5c-48d3-8243-2190311e185e") * 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`](https://github.com/facebook/react-native/blob/be06fd4e22a500128d202436600381b8bc17b3f5/packages/react-native/ReactCommon/react/renderer/uimanager/primitives.h#L120-L121) (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`.

* 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](https://github.com/expo/expo/blob/01429d97ca868ac87bce5e3331a810d85d539603/packages/expo-modules-core/ios/Fabric/ExpoFabricView.swift#L149-L169)). * 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](https://github.com/dominicstop/react-native-ios-utilities/compare/7eef213...ae19904)) * **Attachments**: 🖼️ [Screenshot-2024-04-28-8:17:24-PM](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/294c7bc4-a439-495b-80dd-94b1df9f1974), 🖼️ [Screenshot-2024-04-28-8:17:30-PM](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/d41c746a-4541-4f34-a1ce-98340f3a2e11) * **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](https://github.com/dominicstop/react-native-ios-utilities/compare/ae19904...241a37a)) * **Attachments**: 🖼️ [Screenshot-2024-04-30-3:28:22-PM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/248dd6ab-4832-49e9-8eed-b7b9c8591638), 🖼️ [Screenshot-2024-04-30-3:31:38-PM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/aecec1b2-8055-40d7-850f-c17896878f56) * **Desc**: Finished creating a proof of concept "wrapper" for using a swift view inside fabric. This approach does not use subclassing `RCTComponentView` directly, but rather uses delegation.

* `2024-05-01-00:05 PST` - `react-native-ios-utilities` ([Commits](https://github.com/dominicstop/react-native-ios-utilities/compare/241a37a...51acb57)) * **Attachments**: 🖼️ [Screenshot-2024-04-30-11:58:36-PM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/c25cb395-171c-45fc-b4b1-052ce9c6e06a), 🖼️ [Screenshot-2024-05-01-12:01:06-AM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/a711adb0-cc49-4639-a8c9-dd3762a71244), 🖼️ [Screenshot-2024-05-01-12:02:12-AM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/524fca1d-51b6-4bd9-b71d-a28053111185), 🖼️ [Screenshot-2024-05-01-12:03: 01-AM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/1118b71f-3001-4371-94c2-539d09b81db4) * **Desc**: Impl. `RNIViewLifecycleEventsNotifiable.notifyOnUpdateLayoutMetrics` + `RNILayoutMetrics` to get notified of `RCTComponentViewProtocol.updateLayoutMetrics`.

* `2024-05-01-13:00 PST` - Log * **Attachments**: 🖼️ [Screenshot-2024-05-01-12:54:40-PM.png](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/7dd89669-07d3-410c-9367-32806a92cbaf) * The [docs](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/1118b71f-3001-4371-94c2-539d09b81db4) 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](https://github.com/dominicstop/react-native-ios-utilities/compare/51acb57...e0358ee)) * **Attachments**: 📼 [Screen-Recording-2024-05-02-7:01:45-PM.mov](https://github.com/dominicstop/react-native-ios-context-menu/assets/18517029/76c0f2ca-45ed-490f-8917-d52ee6897dec) * **Desc**: Impl. the ff. `RNIViewLifecycleEventsNotifiable` lifecycle events: `notifyOnFinalizeUpdates`, `notifyOnPrepareForReuse`, `notifyOnMountChildComponentView`, `notifyOnUnmountChildComponentView`.

* `2024-05-23-03:09 PST` - `react-native-ios-utilities` ([Commits](https://github.com/dominicstop/react-native-ios-utilities/compare/e0358ee...f6af55c)) * **Desc**: Implemented common swift API between fabric and paper (e.g. props, `setSize`, events, view commands, etc).