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.

Sign up for GitHub

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

Fix bridgeless for [email protected] #5901

Merged
merged 4 commits into from
Apr 18, 2024
Merged

Fix bridgeless for [email protected] #5901

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b3b419
Fix bridgeless for [email protected]
piaskowyk Apr 15, 2024
097ef62
Merge branch 'main' into @piaskowyk/[email protected]
piaskowyk Apr 17, 2024
32ab312
Upgrade rn to rc.9
piaskowyk Apr 17, 2024
3e54ad0
Fix reload in production app
piaskowyk Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 8 additions & 24 deletions apple/REAModule.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ @implementation REAModule {
SingleInstanceChecker<REAModule> singleInstanceChecker_;
#endif // NDEBUG
bool hasListeners;
bool _isBridgeless;
}

@synthesize moduleRegistry = _moduleRegistry;
Expand Down Expand Up @@ -158,47 +159,35 @@ - (void)handleJavaScriptDidLoadNotification:(NSNotification *)notification

/*
* Taken from RCTNativeAnimatedTurboModule:
* In bridgeless mode, `setBridge` is never called during initialization. Instead this selector is invoked via
* BridgelessTurboModuleSetup.
* This selector is invoked via BridgelessTurboModuleSetup.
*/
- (void)setSurfacePresenter:(id<RCTSurfacePresenterStub>)surfacePresenter
{
_surfacePresenter = surfacePresenter;
_isBridgeless = true;
}

- (void)setBridge:(RCTBridge *)bridge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
// This method isn't called on Bridgeless mode.
[super setBridge:bridge];

[bridge.uiManager.observerCoordinator addObserver:self];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use it on the new architecture

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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).
  2. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// only within the first loading `self.bridge.surfacePresenter` exists
// during the reload `self.bridge.surfacePresenter` is null
_surfacePresenter = self.bridge.surfacePresenter;
if (self.bridge.surfacePresenter) {
_surfacePresenter = self.bridge.surfacePresenter;
}

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

[self setNodesManager:self.bridge];
}
_nodesManager = [[REANodesManager alloc] initWithModule:self bridge:bridge surfacePresenter:_surfacePresenter];

- (void)initialize
{
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(handleJavaScriptDidLoadNotification:)
name:RCTJavaScriptDidLoadNotification
object:nil];

[[self.moduleRegistry moduleForName:"EventDispatcher"] addDispatchObserver:self];

// [bridge.uiManager.observerCoordinator addObserver:self]; // TODO: Check if it's needed on new arch.
piaskowyk marked this conversation as resolved.
Show resolved Hide resolved
#ifndef NDEBUG
[self setReaSurfacePresenter];
#endif // NDEBUG
Comment on lines -197 to -199
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


[self setNodesManager:nil];
}

#ifndef NDEBUG
Expand All @@ -213,11 +202,6 @@ - (void)setReaSurfacePresenter
}
#endif // NDEBUG

- (void)setNodesManager:(RCTBridge *)bridge
{
_nodesManager = [[REANodesManager alloc] initWithModule:self bridge:bridge surfacePresenter:_surfacePresenter];
}

#else // RCT_NEW_ARCH_ENABLED

- (void)setBridge:(RCTBridge *)bridge
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 💀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#if REACT_NATIVE_MINOR_VERSION >= 74 && defined(RCT_NEW_ARCH_ENABLED)
RCTCxxBridge *cxxBridge = (RCTCxxBridge *)[RCTBridge currentBridge];
auto &rnRuntime = *(jsi::Runtime *)cxxBridge.runtime;
Expand Down
Loading