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

feat: make RNScreens compatible with bridgeless mode #1990

Closed
wants to merge 3 commits into from

Conversation

WoLewicki
Copy link
Member

Description

PR enabling bridgeless mode in FabricTestExample so we can check if the library is compatible with it. The only problem I encountered was the usage of currentBridge which now returns nil on iOS. I made a dirty hack to get the needed module from some private fields. It would be nice to find a better solution for this, but we have to start with something 🚀

Changes

  • Provide a bridgeless-compatible solution for getting the event dispatcher.

Test code and steps to reproduce

Test1802.tsx where we use the headerHeight event with Animated. For now you have to comment the code using RNGH and Reanimated to test it since they are not yet compatible with bridgeless mode.

@kkafar kkafar added Feature impl PR with feature implementation and removed Feature impl PR with feature implementation labels Dec 14, 2023
Comment on lines +320 to +321
RCTModuleRegistry *moduleRegistry =
(RCTModuleRegistry *)[[appDelegate valueForKey:@"_reactHost"] valueForKey:@"_moduleRegistry"];
Copy link

@RSNara RSNara Dec 14, 2023

Choose a reason for hiding this comment

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

@WoLewicki, this is very interesting!

I don't understand the full code. But, why do we have to rely on the bridge/module registry for this use-case? Do you think there's any way to re-structure this so that we don't have to rely on the bridge or the module registry?

I'm asking to know if we need to provide a better api for this. (We definitely shouldn't be doing [[appDelegate valueForKey:@"_reactHost"] valueForKey:@"_moduleRegistry"]).

Copy link
Member Author

Choose a reason for hiding this comment

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

What we need here is the RCTImageLoader module so if there is another way to get it then we could remove it from here for sure.

Copy link

Choose a reason for hiding this comment

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

So, from what I can gather:

  • RNSScreenStackHeaderConfig is a component.
  • And it needs access to a native module: RCTImageLoader.

As far as I'm aware, fabric components do not have access to native modules. (I think this was also the case with legacy components). And... I think there's been some hesitation about giving components access to native modules.

Let me follow up internally!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks for information and waiting for follow up 🚀

Copy link

@RSNara RSNara Jan 9, 2024

Choose a reason for hiding this comment

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

@WoLewicki, the recommended approach is to follow the footsteps of the < Image/> component.

Create a hand-written ComponentDescriptor. ComponentDescriptors have access to the contextContainer. The contextContainer contains the RCTImageLoader.

https://github.com/facebook/react-native/blob/f7f9250f6e90a41e798a8f62738925a2859a751a/packages/react-native/ReactCommon/react/renderer/components/image/ImageComponentDescriptor.h#L20-L39

You'll also need to write a custom ShadowNode. The component descriptor will attach some c++ object to the shadow node that calls into [RCTImageLoader imageForUrl...]. And this Fabric component will be given access to the shadow node. From that shadow node, it can grab that c++ object and call the method that eventually triggers [RCTImageLoader imageForUrl...].

Comment on lines +107 to +123
- (void)dispatchEventForAnimatedObserver:(id<RCTEvent>)event
{
auto eventDispatcher = [[RCTBridge currentBridge] eventDispatcher];
if (eventDispatcher) {
[eventDispatcher sendEvent:event];
} else {
id appDelegate = [[UIApplication sharedApplication] delegate];
RCTModuleRegistry *moduleRegistry =
(RCTModuleRegistry *)[[appDelegate valueForKey:@"_reactHost"] valueForKey:@"_moduleRegistry"];
if (moduleRegistry) {
id<RCTEventDispatcherProtocol> legacyEventDispatcher = [moduleRegistry moduleForName:"EventDispatcher"
lazilyLoadIfNecessary:YES];

[legacyEventDispatcher notifyObserversOfEvent:event];
}
}
}
Copy link

@RSNara RSNara Jan 9, 2024

Choose a reason for hiding this comment

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

@WoLewicki, the event dispatcher just calls a javascript module method.

Instead of using the event dispatcher, could you instead dispatch a fabric event? And then, on the javascript side, if necessary, just call the JavaScript module there?

@WoLewicki
Copy link
Member Author

Closing this PR in favor of changes that are already on main.

@WoLewicki WoLewicki closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature impl PR with feature implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants