Fix bridgeless for [email protected] #5901

piaskowyk · 2024-04-15T13:18:12Z

Summary

This pull request adjusts the Reanimated initialisation process to changes in React Native versions between 0.74.0-rc.4 and 0.74.0-rc.8. Now, the method setBridge is called in both bridgeless and bridgefull modes.

piaskowyk · 2024-04-15T13:19:04Z

apple/REAModule.mm

  [super setBridge:bridge];
-
-  [bridge.uiManager.observerCoordinator addObserver:self];


We don't use it on the new architecture

I was wondering if we use it in RNGH to pass events to Reanimated but turns out we don't (in RNGH there's a function sendEventForReanimated that on new arch calls reanimatedEventDispatcher.sendEvent() which in turn calls reanimatedModule?.nodesManager?.onEventDispatch(event) so I guess that's okay).

If it's not needed on the new architecture, let's wrap <RCTEventDispatcherObserver> in header file and eventDispatcherWillDispatchEvent: method inside #ifdef RCT_NEW_ARCH_ENABLED, what do you think?

As we discussed offline, we refactor the whole initialization process in the following PR.

apple/REAModule.mm

WoLewicki

Hope it works now 🎉

tomekzaw · 2024-04-16T06:08:41Z

apple/REAModule.mm

  [super setBridge:bridge];
-
-  [bridge.uiManager.observerCoordinator addObserver:self];


I was wondering if we use it in RNGH to pass events to Reanimated but turns out we don't (in RNGH there's a function sendEventForReanimated that on new arch calls reanimatedEventDispatcher.sendEvent() which in turn calls reanimatedModule?.nodesManager?.onEventDispatch(event) so I guess that's okay).

If it's not needed on the new architecture, let's wrap <RCTEventDispatcherObserver> in header file and eventDispatcherWillDispatchEvent: method inside #ifdef RCT_NEW_ARCH_ENABLED, what do you think?

tomekzaw · 2024-04-16T06:15:13Z

apple/REAModule.mm

 */
 - (void)setSurfacePresenter:(id<RCTSurfacePresenterStub>)surfacePresenter
 {
  _surfacePresenter = surfacePresenter;
+  _isBridgeless = true;
 }

 - (void)setBridge:(RCTBridge *)bridge


It's a bit confusing that we have two separate places where we define setBridge: method (line 170 and line 207). I know it was like that prior to this PR. It's definitely not a blocker but we should do something about it in a separate PR.

tomekzaw · 2024-04-16T06:20:22Z

apple/REAModule.mm

-#ifndef NDEBUG
-  [self setReaSurfacePresenter];
-#endif // NDEBUG


I wonder why is this code DEBUG-only? It was like that before, just curious to know if that's right.

It shouldn't be 🫣 I fixed it in 3e54ad0. Also. We still need REAInitializerRCTFabricSurface class for the Fabric with bridge :/

tsapeta · 2024-04-16T17:48:35Z

apple/REAModule.mm

@@ -295,7 +279,7 @@ - (void)sendEventWithName:(NSString *)eventName body:(id)body

 RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD(installTurboModule : (nonnull NSString *)valueUnpackerCode)
 {
-  if (!self.bridge) {
+  if (_isBridgeless) {


Why do you need a separate implementation for bridgeless here?

The implementation for non-bridgeless looks good to me, just don't use [self.bridge respondsToSelector:@selector(runtime)] check – it no longer returns YES even if the runtime is there. The reason is that the bridge is now an instance of RCTBridgeProxy that inherits from NSProxy which probably has a custom implementation for respondsToSelector, different than NSObject.
We know that bridge.runtime has been available (as long as you have a category interface) for many RN versions back, so it seems to be safe to call it.

Two more nitpicks regarding the implementation for bridgeless:

Using [RCTBridge currentBridge] is very risky and I guess pretty easy to break in an app with multiple bridges (or now React instances).

bridge.jsCallInvoker was initially not available in the bridge proxy, but it is now. I think there is no need to use the runtime executor in its current shape (call invoker, runtime executor and runtime scheduler might be unified in the next RN versions).

bridge.jsCallInvoker was initially not available in the bridge proxy, but it is now. I think there is no need to use the runtime executor in its current shape (call invoker, runtime executor and runtime scheduler might be unified in the next RN versions).

Are you telling me that all our work on refactoring Reanimated initialization pipeline to be able to dynamically utilize either jsCallInvoker or runtimeExecutor was mostly pointless? 💀

Yes xD

Maybe not entirely pointless, because the runtime executor might become the recommended way when the RN team figures out the final API for libraries. Right now we have two ways and both are lacking of features (e.g. synchronous executions), so I would recommend to use the call invoker and don't complicate the code too much until then.

tjzel

🙏

FabricExample/ios/FabricExample.xcodeproj/project.pbxproj

## Summary This PR fixes production build for iOS. In #5901 I called `setReaSurfacePresenter` method but this one was defined only on debug build. Fixes #5912 ## Test plan Run production build for iOS

## Summary This PR adds missing registration for platform events such as scroll events on Bridgeless mode. In the previous PR (#5901) I checked support for Gesture Handler events, I forgot about platform events ## Test plan | before | after | | --- | --- | | <video src="https://github.com/software-mansion/react-native-reanimated/assets/36106620/0d83f08a-16d4-4669-bfea-a630ded6451f" /> | <video src="https://github.com/software-mansion/react-native-reanimated/assets/36106620/2819b8b8-8c0a-4160-80d1-c5488ea9b0a5" /> |

## Summary This PR fixes production build for iOS. In #5901 I called `setReaSurfacePresenter` method but this one was defined only on debug build. Fixes #5912 ## Test plan Run production build for iOS

## Summary This PR adds missing registration for platform events such as scroll events on Bridgeless mode. In the previous PR (#5901) I checked support for Gesture Handler events, I forgot about platform events ## Test plan | before | after | | --- | --- | | <video src="https://github.com/software-mansion/react-native-reanimated/assets/36106620/0d83f08a-16d4-4669-bfea-a630ded6451f" /> | <video src="https://github.com/software-mansion/react-native-reanimated/assets/36106620/2819b8b8-8c0a-4160-80d1-c5488ea9b0a5" /> |

…5953) ## Summary As I pointed out [here](#5901 (comment)), using `[RCTBridge currentBridge]` may lead to the wrong bridge in apps that have multiple React instances, as is the case for expo-dev-client. https://github.com/software-mansion/react-native-reanimated/blob/7e1a19ed8190763456e96939eec71c2528c1eed4/apple/REAModule.mm#L284 Fixes #5497 Instead of using `[RCTBridge currentBridge]` we can just call `self.bridge` and ensure the correct bridge is always used ## Test plan Run FabricExample app on iOS Co-authored-by: Krzysztof Piaskowy <[email protected]>

Fix bridgeless for [email protected]

7b3b419

piaskowyk requested review from tomekzaw, WoLewicki and tjzel April 15, 2024 13:18

piaskowyk commented Apr 15, 2024

View reviewed changes

apple/REAModule.mm Show resolved Hide resolved

WoLewicki approved these changes Apr 15, 2024

View reviewed changes

tomekzaw reviewed Apr 16, 2024

View reviewed changes

tsapeta reviewed Apr 16, 2024

View reviewed changes

piaskowyk added 3 commits April 17, 2024 11:47

Merge branch 'main' into @piaskowyk/[email protected]

097ef62

Upgrade rn to rc.9

32ab312

Fix reload in production app

3e54ad0

tjzel approved these changes Apr 17, 2024

View reviewed changes

FabricExample/ios/FabricExample.xcodeproj/project.pbxproj Show resolved Hide resolved

piaskowyk enabled auto-merge April 18, 2024 09:43

piaskowyk disabled auto-merge April 18, 2024 09:45

piaskowyk merged commit 25bd0b9 into main Apr 18, 2024
14 checks passed

piaskowyk deleted the @piaskowyk/[email protected] branch April 18, 2024 09:45

timbocole mentioned this pull request Apr 18, 2024

react-native-reanimated 3.9.0-rc.1 fails to build on iOS with new architecture enabled in Release build #5912

Closed

piaskowyk mentioned this pull request Apr 22, 2024

Fix production build #5925

Merged

piaskowyk mentioned this pull request Apr 26, 2024

Fix platform events for bridgeless #5937

Merged

tsapeta mentioned this pull request Apr 29, 2024

Crash with expo custom build (49.x or 50.x, or 51.x) when newArchEnabled set to true on iOS and with expo-dev-client installed #5497

Closed

gabrieldonadel mentioned this pull request Apr 29, 2024

Prevent iOS bridgeless initialization from getting the wrong bridge #5953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bridgeless for [email protected] #5901

Fix bridgeless for [email protected] #5901

piaskowyk commented Apr 15, 2024

piaskowyk Apr 15, 2024

tomekzaw Apr 16, 2024

piaskowyk Apr 17, 2024

WoLewicki left a comment

tomekzaw Apr 16, 2024

tomekzaw Apr 16, 2024

tomekzaw Apr 16, 2024

piaskowyk Apr 17, 2024

tsapeta Apr 16, 2024

tjzel Apr 17, 2024

tsapeta Apr 17, 2024

tjzel left a comment

		[super setBridge:bridge];

		[bridge.uiManager.observerCoordinator addObserver:self];

Fix bridgeless for [email protected] #5901

Fix bridgeless for [email protected] #5901

Conversation

piaskowyk commented Apr 15, 2024

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WoLewicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjzel left a comment

Choose a reason for hiding this comment