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

[Bridgeless][IOS] bridge is nil on Swift when NewArch is enabled #45232

Open
lodev09 opened this issue Jul 1, 2024 · 19 comments
Open

[Bridgeless][IOS] bridge is nil on Swift when NewArch is enabled #45232

lodev09 opened this issue Jul 1, 2024 · 19 comments
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@lodev09
Copy link

lodev09 commented Jul 1, 2024

Description

I maintain a native component library and trying to make it work with New Architecture interop. It builds without problem. However, the bridge in the view manager instance is nil and causing the component to crash.

To give context, the library is similar to RN Modal where it uses the view component as a host view of a view controller. It needs the bridge to attach RCTTouchHandler to the container view. The library is written in swift. https://github.com/lodev09/react-native-true-sheet

Any ideas or I'm missing something?

Steps to reproduce

  1. Clone the library
  2. Modify example/app.json and set newArchEnabled: true at line 38
  3. Run the example project
  4. Notice the crash

React Native Version

0.74.2

Affected Platforms

Runtime - iOS

Areas

Bridgeless - The New Initialization Flow

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU: (10) arm64 Apple M1 Pro
  Memory: 355.75 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.20.3
    path: ~/.nvm/versions/node/v18.20.3/bin/node
  Yarn:
    version: 4.2.2
    path: ~/.nvm/versions/node/v18.20.3/bin/yarn
  npm:
    version: 10.8.1
    path: ~/.nvm/versions/node/v18.20.3/bin/npm
  Watchman: Not Found
Managers:
  CocoaPods:
    version: 1.15.2
    path: /opt/homebrew/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.2 AI-232.10300.40.2321.11668458
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.10
    path: /usr/bin/javac
  Ruby:
    version: 3.0.6
    path: /Users/lodev09/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.2
    wanted: 0.74.2
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

TrueSheet/TrueSheetViewManager.swift:22: Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value

Reproducer

https://github.com/lodev09/react-native-true-sheet/tree/main/example

Screenshots and Videos

Screenshot 2024-07-01 at 22 03 16
@lodev09 lodev09 added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Jul 1, 2024
@github-actions github-actions bot added the Platform: iOS iOS applications. label Jul 1, 2024
@lodev09 lodev09 changed the title [Bridgeless][IOS] bridge is nil on Swift when NewArch is enabled [Bridgeless][IOS] bridge is nil on Swift when NewArch is enabled Jul 1, 2024
@cortinico cortinico added Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. and removed Needs: Triage 🔍 labels Jul 1, 2024
@cipolleschi
Copy link
Contributor

Hi @lodev09, thanks for maintaining a library for React Native and thank you for the issue.
As the name implies, the Bridgless mode means No Bridge. So it makes sense that the bridge instance is actually nil: no bridge is actually initialized.

There are two questions that I need to ask you:

  1. Are you migrating the component to Fabric?
  2. Are you trying to use the component throught the Interop Layer?

My guess is that nobody is setting the bridge in the ViewManager, after it is created: in bridgeless mode, that's kind of expected, given that there is no bridge. But in theory it should work thanks to the Interop Layer.

I'll have a look at it later today or tomorrow.

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

As the name implies, the Bridgless mode means No Bridge. So it makes sense that the bridge instance is actually nil: no bridge is actually initialized.

Hmmm, interesting. I'm wondering how the RN Modal works on bridgeless then? It uses exactly the same code to set the TouchHandler -- the only difference is that my library is written in swift.

  1. Are you migrating the component to Fabric?
  2. Are you trying to use the component throught the Interop Layer?

I'm trying to run it through Interop layer first and then ultimately fabric. As I mentioned, everything works fine aside from the bridge being nil.

I just need the TouchHandler to work on New Arch :/

@cipolleschi
Copy link
Contributor

I'm wondering how the RN Modal works on bridgeless then?

RN Modals are actually already migrated to Fabric. When you run the new architecture, the component that is loaded is this one.

@cipolleschi
Copy link
Contributor

Ok, I got the issue. I'll work on a fix as there could be other libraries that rely on the bridge property to be set and it is currently not set in the interop layer.

I hope I'll be able to send the fix out before EOW.

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

Okay.. thank you so much @cipolleschi 🙏

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

Quick question: If I understand correctly, swift is not yet supported for Fabric components right?

cipolleschi added a commit to cipolleschi/react-native that referenced this issue Jul 8, 2024
Summary:
Thanks to [facebook#45232](facebook#45232) we found a bug in the interop layer, where we were not passing the BridgeProxy in bridgeless mode to the view managers.

This Change should fix that issue.

## Changelog:
[iOS][Fixed] - Make sure to pass the RCTBridgeProxy to  ViewManagers

Differential Revision: D59468292
@cipolleschi
Copy link
Contributor

swift is not yet supported for Fabric components right?

Tricky question!
Technically, no, it is not supported. What we mean with that is that you can't create a Fabric component using only Swift.

However, you can create a thin wrapper in objective-c++ and then forward the various calls to Swift. We haven't experimented with that setup a lot, and we are aware that there are some things that are not compatible and that will require some wrapping to make them work in Swift.

@cipolleschi
Copy link
Contributor

So.. #45329 should fix the crash. But, from what I am seeing, the buttons are not tappable... :/

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

Yeah.. I use the bridge to mainly setSize and create an RCTTouchHandler instance to be attached to the container view. I'm hoping this is supported on the Interop layer.

https://github.com/lodev09/react-native-true-sheet/blob/643f4e8f39aabfe1403de74042b25bdb392f18ce/ios/TrueSheetView.swift#L59

@cipolleschi
Copy link
Contributor

I need to ask around and investigate... I'll keep you posted!

@cipolleschi
Copy link
Contributor

That was quick... xD
I found that:

I made a quick experiment in TrueSheetView.swift:

// ...
class TrueSheetView: UIView, RCTInvalidating, TrueSheetViewControllerDelegate {
//...
  private var touchHandler: RCTTouchHandler
+  private var surfaceTouchHandler: RCTSurfaceTouchHandler
// ..
init(with bridge: RCTBridge) {
// ...
    touchHandler = RCTTouchHandler(bridge: bridge)
+    surfaceTouchHandler = RCTSurfaceTouchHandler()
//...

override func insertReactSubview(_ subview: UIView!, at index: Int) {
// ...

    containerView = subview
-    touchHandler.attach(to: containerView)
+    touchHandler.attach(to: subview)
+    surfaceTouchHandler.attach(to: subview)
  }
//..
  override func removeReactSubview(_ subview: UIView!) {
 //...

    touchHandler.detach(from: subview)
+    surfaceTouchHandler.detach(from: subview)

//...

And, of course, adding the following in the bridging header

#import <React/RCTTouchHandler.h>
+ #import <React/RCTSurfaceTouchHandler.h>
#import <React/RCTScrollView.h>
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-08.at.18.25.59.mp4

sizings looks a bit off, though... :(

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

Wow thank you! :D
Just need a new arch condition and should be go to go with the touch handler!

sizings looks a bit off, though... :(

Yeah, looks like bridge.uiManager.setSize isn't working... is there any alternative? 😅
Code is here:
https://github.com/lodev09/react-native-true-sheet/blob/643f4e8f39aabfe1403de74042b25bdb392f18ce/ios/TrueSheetView.swift#L147-L152

@cipolleschi
Copy link
Contributor

It turns out, that method was not implemented in the interop layer for Bridgeless. I'll try to work on that tomorrow!

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

You're awesome @cipolleschi. Thank you so much!

@cipolleschi
Copy link
Contributor

cipolleschi commented Jul 8, 2024

Thank you for reporting the issue and for providing a way for me to work on that!

(Anyway... start thinking about migrating it to Fabric! 😅 It will be needed, at some point! Happy to help there as well)

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

@cipolleschi yes, I'm really eager to migrate it to fabric -- it's just a bummer that swift isn't supported out of the box. But we'll get there 💪

@lodev09
Copy link
Author

lodev09 commented Jul 8, 2024

This is my first native ios/android project so I'm not 100% familiar with all this yet 😅

facebook-github-bot pushed a commit that referenced this issue Jul 8, 2024
…er (#45329)

Summary:
Pull Request resolved: #45329

Thanks to [#45232](#45232) we found a bug in the interop layer, where we were not passing the BridgeProxy in bridgeless mode to the view managers.

This Change should fix that issue.

## Changelog:
[iOS][Fixed] - Make sure to pass the RCTBridgeProxy to  ViewManagers

Reviewed By: dmytrorykun

Differential Revision: D59468292

fbshipit-source-id: 00666be21385a735878eb567c4b8a0986c609c5f
@cipolleschi
Copy link
Contributor

@lodev09 I looked into this issue a little bit more.
The method that is missing is RCTUIManager setSize:forView:.

The reason why it hasn't been implemented in the Interop Layer is architectural.
In the Old Architecture, we have a native layer that creates a tree of views. Every view is backed by a shadow view that is used to compute the layout. The JS React tree talks to the platform and creates the Native tree and the Shadow tree at the same time. The old RCTUIManager has a mapping between native view and shadow view.
The native layer is the source of truth.

In the New Architecture, we have a shadow tree implementation in C++. In the New Architecture, the Shadow Tree drives everything and it creates the Native tree. The Native views don't have a handle to get the shadow views anymore and they can't really change the layout from the Native layer.
The shadow layer is the source of truth.

Due to this, we cannot really port the setSize method back to the Interop Layer.
I searched on Github and there are not a lot of libraries that uses that method (< 1% of all the React Native libraries) so it does not really make sense to support this API due to its architectural incompatibility with the New Architecture and its low usage.

My suggestion would be to migrate your library to Fabric properly and support both architectures.

We have some guides on how to do it:

And you can use the New Architecture implementation of the React Native Modal component as base implementation for the Fabric version of it.

I would also love to help with review and suggestions on how to bridge ObjC++ with Swift. I made some experiments with modules in the past, here and here, but I never had the time to try with Componentes.

I hope these pointers can help you move forward with this.

@lodev09
Copy link
Author

lodev09 commented Jul 9, 2024

@cipolleschi I see. I'm going to implement an alternative approach to updating the size as a short term solution for now, maybe in the JS side.

I agree with migrating to Fabric for the long term. I might just convert it all to ObjC 😅

Anyways, thanks again for your help!

blakef pushed a commit that referenced this issue Jul 15, 2024
…er (#45329)

Summary:
Pull Request resolved: #45329

Thanks to [#45232](#45232) we found a bug in the interop layer, where we were not passing the BridgeProxy in bridgeless mode to the view managers.

This Change should fix that issue.

## Changelog:
[iOS][Fixed] - Make sure to pass the RCTBridgeProxy to  ViewManagers

Reviewed By: dmytrorykun

Differential Revision: D59468292

fbshipit-source-id: 00666be21385a735878eb567c4b8a0986c609c5f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

3 participants