forked from facebook/react-native
-
Notifications
You must be signed in to change notification settings - Fork 0
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
RCTScrollView section headers issue. #3
Comments
Merged! |
lzcheung
pushed a commit
that referenced
this issue
Nov 14, 2019
…to match Android Summary: ## Overview This diff switches the RCTSlider onSlidingComplete event on iOS from a bubbling event to a direct (non-bubbling) event to match the non-bubbling type on android. Note that in this case these seems like a bug. I will still explain the motivation and reasoning as this will come up in future diffs. ## Changelog [Slider][BREAKING] Switch Slider onSlidingComplete event to a non-bubbling event on iOS to match Android ## Motivation: The motivation here is that when we codgen the view configs, we'll need to unify all of the events and props across platforms for components that have the same name on iOS and Android. In this case, the view configs (below) conflict for onSlidingComplete. On iOS this is under bubblingEventTypes, on Android this is under directEventTypes. We have code [here](https://fburl.com/3s1dahm2) in the react native renderer which ensures an event is not listed as both. ``` // iOS const SliderViewConfig = { bubblingEventTypes: { onSlidingComplete: { phasedRegistrationNames: { captured: 'onChangeCapture', bubbled: 'onChange' } } }, directEventTypes: { // None }, validAttributes: { // ... } }; ``` ``` // Android const SliderViewConfig = { bubblingEventTypes: { // None }, directEventTypes: { onSlidingComplete: { registrationName: 'onEventDirect' } }, validAttributes: { // ... } }; ``` ## Solutions There are three solutions to this issue: 1. Don't generate view configs 2. Rename the component on one platform 3. Make a breaking change in the event behavior on one platform to make it consistent across both platforms Here we've chosen option #3 Reviewed By: TheSavior Differential Revision: D15322304 fbshipit-source-id: ff1ab86efe9e2bc50fd3f7619e6760ab5c1c5090
MattFoley
pushed a commit
that referenced
this issue
Jul 5, 2023
Summary: This fix is still a little hypothetical. We have a few different JS errors that we're seeing with bridgeless mode that seem to be caused by Fabric trying to access `__fbBatchedBridge` from C++. I think what's happening is: 1. User encounters an unrelated JS error very early in rendering a new surface (possibly while the bundle is still loading?) 2. In release builds, BridgelessReactFragment handles the error by stopping the surface and rendering a retry button (actually, the surface is stopped in a bunch of places in BaseFbReactFragment, which might be why this is popping up now - I recently refactored that class to share more of its logic in bridgeless mode) 3. Fabric stops the surface by first checking to see if the custom binding `RN$stopSurface` exists; if not, it falls back to calling the registered callable module `ReactFabric`. I think #3 is where things are going wrong for bridgeless mode; if you call stopSurface before `RN$stopSurface` is installed (which happens when ReactFabric shim is required) then you'll fall back to the bridge version. My solution here is to instead rely on a flag set in C++ to determine whether we're in bridgeless mode, and then check to see if the stopSurface binding has been installed. If not, we just noop - if the ReactFabric shim hasn't been required, we probably don't actually have a React surface that needs to be stopped. At least, that's my current theory. We'll see if this actually works. Changelog: [Fixed][iOS] Fix an issue calling stopSurface in bridgeless mode before surface is started Reviewed By: mdvacca Differential Revision: D25453696 fbshipit-source-id: bff76675c43989101d0ba5ae0aba60089db230bf
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Resolve once facebook#2224 is merged.
The text was updated successfully, but these errors were encountered: