Summary:
In React Native there are three types of "Native" components.
```
createReactClass with NativeMethodsMixin
```
```
class MyComponent extends ReactNative.NativeComponent
```
```
requireNativeComponent('RCTView')
```
The implementation for how to handle all three of these exists in the React Native Renderer. Refs attached to components created via these methods provide a set of functions such as
```
.measure
.measureInWindow
.measureLayout
.setNativeProps
```
These methods have been used for our core components in the repo to provide a consistent API. Many of the APIs in React Native require a `reactTag` to a host component. This is acquired by calling `findNodeHandle` with any component. `findNodeHandle` works with the first two approaches.
For a lot of our new Fabric APIs, we will require passing a ref to a HostComponent directly instead of relying on `findNodeHandle` to tunnel through the component tree as that behavior isn't safe with React concurrent mode.
The goal of this change is to enable us to differentiate between components created with `requireNativeComponent` and the other types. This will be needed to be able to safely type the new APIs.
For existing components that should support being a host component but need to use some JS behavior in a wrapper, they should use `forwardRef`. The majority of React Native's core components were migrated to use `forwardRef` last year. Components that can't use forwardRef will need to have a method like `getNativeRef()` to get access to the underlying host component ref.
Reviewed By: rickhanlonii
Differential Revision: D17563615
fbshipit-source-id: b9e6042805517d502770fcba37301c2c5b6452b6
Summary:
We need to migrate to HostComponent instead of the exported type from codegenNativeComponent which is the same type
Changelog:
[Internal] Migrate NativeComponentType from codegenNativeComponent to HostComponent
Reviewed By: rickhanlonii
Differential Revision: D17563307
fbshipit-source-id: 01c8fea8c67b33bed42ae28ffb8c132be87b9a7a
Summary:
We need to migrate to HostComponent, this is the first batch.
Changelog:
[Internal] Migrate NativeComponentType from codegenNativeComponent to HostComponent
Reviewed By: rickhanlonii
Differential Revision: D17562879
fbshipit-source-id: ce1993b64a79cede3598c89ddff0dadf07fde92f
Summary:
In React Native there are three types of "Native" components.
```
createReactClass with NativeMethodsMixin
```
```
class MyComponent extends ReactNative.NativeComponent
```
```
requireNativeComponent('RCTView')
```
The implementation for how to handle all three of these exists in the React Native Renderer. Refs attached to components created via these methods provide a set of functions such as
```
.measure
.measureInWindow
.measureLayout
.setNativeProps
```
These methods have been used for our core components in the repo to provide a consistent API. Many of the APIs in React Native require a `reactTag` to a host component. This is acquired by calling `findNodeHandle` with any component. `findNodeHandle` works with the first two approaches.
For a lot of our new Fabric APIs, we will require passing a ref to a HostComponent directly instead of relying on `findNodeHandle` to tunnel through the component tree as that behavior isn't safe with React concurrent mode.
The goal of this change is to enable us to differentiate between components created with `requireNativeComponent` and the other types. This will be needed to be able to safely type the new APIs.
For existing components that should support being a host component but need to use some JS behavior in a wrapper, they should use `forwardRef`. The majority of React Native's core components were migrated to use `forwardRef` last year. Components that can't use forwardRef will need to have a method like `getNativeRef()` to get access to the underlying host component ref.
Note, we will need follow up changes as well as changes to the React Renderer in the React repo to fully utilize this new type.
Changelog:
[Internal] Flow type to differentiate between HostComponent and NativeMethodsMixin and NativeComponent
Reviewed By: jbrown215
Differential Revision: D17551089
fbshipit-source-id: 7a30b4bb4323156c0b2465ca41fcd05f4315becf
Summary:
In D17439957, I noted that YogaLogger#log throws a NoMethodFoundException when called from C++ b/c C++ and Java's signatures of that method don't match. C++ uses YogaNodeJNIBase for the first param, Java uses YogaNode. Both my attempts to fix this failed.
Attempt #1 - Make Java use YogaNodeJNIBase. This doesn't work because the :java-interface target includes YogaLogger but not YogaNodeJNIBase. Moving YogaLogger to the impl target doesn't work either b/c other files in :java-interface reference YogaLogger.
Attempt #2 - Make C++ use YogaNode. This doesn't work b/c we try to call the log method with objects of fbjni type YogaNodeJNIBase. This would be fine in Java since YogaNodeJNIBase extends YogaNode. But fbjni's typing isn't advanced enough to know this, so the Yoga C++ fails to compile.
At this point, I was wondering what the value of having this param in the log function at all was. None of the implementations in our codebase use it today. It might be easier to just remove it all together. This also removes a bug with YGNodePrint where we pass a null layout context that eventually causes a SIG_ABRT when we use it to try to find a YogaNode to pass to this function. (https://fburl.com/diffusion/ssw9h8lv).
Reviewed By: amir-shalem
Differential Revision: D17470379
fbshipit-source-id: 8fc2d95505971a52af2399a9fbb60b63f27f0ec2
Summary:
These types aren't robust to changes in the React component type. When we refactor requireNativeComponent these will error. This change is forwards compatible.
Changelog:
[Internal] Improve internal type in DrawerLayoutAndroid
Reviewed By: JoshuaGross
Differential Revision: D17561194
fbshipit-source-id: 470289449b4d5b3148692f1945fb720e1e3972eb
Summary:
This is used by Image.android.js and needs to be flow typed to be able to have confidence in the requireNativeComponent type change
Changelog:
[Internal] Flow type vendor/core/merge.js
Reviewed By: JoshuaGross
Differential Revision: D17561195
fbshipit-source-id: 2639f2628e15b2dd5469bb2ebfe935a444025a21
Summary:
It's possible that this crashes because the callback is non-null/non-undefined but isn't a function; if so, I would like to collect that information.
These changes have already been made in the React repo.
Reviewed By: TheSavior
Differential Revision: D17559022
fbshipit-source-id: a0538d533c3c482d27eef0ed3c8c980e2bc8e817
Summary:
@public
We're seeing crashes from multiple threads trying to call `[NSData appendData:]` at the same time. Usually the RCTURLRequestHandlers implementation avoids this but if you're using a background queue, it is pretty easy to reach this case.
Adding a lock to accessors of `_data` should prevent this.
Reviewed By: shergin
Differential Revision: D17552136
fbshipit-source-id: 3384d36221d0ada8cda638ad8e79e1bf3862f93f
Summary: Adding `DoNotStrip` to all the interfaces that extend `JavaScriptModule` to ensure they don't get stripped from release builds (because they have no Java implementors).
Reviewed By: emma0303
Differential Revision: D17534719
fbshipit-source-id: a793764caf17040bf1252be7ec4c72176d6989d4
Summary: Another attempt at D17282188, which got partially reverted in D17505827 due to a crash in release builds.
Reviewed By: RSNara
Differential Revision: D17512419
fbshipit-source-id: a1b0abfed2c4a1f3f02da85e84abee0127b1a7e2
Summary:
`addObserver` and `removeObserver` now accepts const references instead of pointers which indicates the intent (non-nullability and non-owning) clearly. The delegate methods are also marked as `const` to designate the possible concurrent execution (`const` means "thread-safe" here).
All changes are pure syntactical, nothing really changes (besides the fact overall code quality and redability).
Reviewed By: JoshuaGross
Differential Revision: D17535395
fbshipit-source-id: b0c6c872d44fee22e38fd067ccd3320e7231c94a
Summary:
# A race condition
The practical thing of this diff is fixing a data race.
Imagine a case where a thread A calls `addObserver` and thread B calls `nativeImageResponseFailed` at the same time.
Thread A might read `status_` exclusively and store result as a local variable and then go sleep.
Then thread B starts and finishes `nativeImageResponseFailed`, it writes `status_` and notifies all observers.
Then thread B wakes up. It adds an observer to a collection of observers and finishes.
As a result, the observer from `addObserver` will never be called.
To fix this, we changed a logic a bit to lock only once per method. During the lock, we read and/or write to storage and then perform side-effects.
In contrast, previously we often locked only around the access of a particular instance variable (several times per method).
The challenge here is that idiomatic/fancy to C++/STL ways to lock mutexes don't work in our case.
# C++ idioms and readability, multiple locks for the same transaction
STL has tools to avoid calling `lock` and `unlock` methods manually (std::lock_guard<> and lamdas). Unfortunately, using that in our use case is quite problematic. That's probably possible but will lead to much less readable code and some copy-pasta in `addObserver`.
Therefore we replaced using `std::lock_guard` with simple `lock` and `unlock` where using `std::lock_guard` was problematic.
# Why we changed `shared_mutex` to a normal one?
After consolidating the locks we found that we have an only case where we can use shared lock (in `nativeImageResponseProgress`). Calling this method in real life is not concurrent, so it makes sense to replace a shared lock with a more simple and performant regular one.
Reviewed By: sammy-SC
Differential Revision: D17368739
fbshipit-source-id: 61d66fb737d8c2dc73001a80a31edaa59a16d886
Summary:
This diff contains some small improvements in `ImageResponseObserverCoordinator` which are pretty minor:
* Now we use `small_vector` instead of a normal one. In the vast majority of cases, the coordinator has only one observer, so having `small_vector` with default size `1` saves us a memory allocation (which is a dozen of allocations for a screen, not bad).
* Empty constructor and destructor were removed.
* Unnecessary copying of ImageResponse was removed. ImageResponse is a practically a shared_pointer, it has value semantic and does not need to be copied. We don't need to acquire mutex to access that.
Reviewed By: sammy-SC
Differential Revision: D17368740
fbshipit-source-id: 828e27a72b9c8ac0063c5fbda00f83ddb255309c
Summary: Just need to validate the intended fix properly via simple gating mechanism.
Reviewed By: mmmulani
Differential Revision: D17536264
fbshipit-source-id: 92db4156beabd6dec2a71b6ea7c2d7bf708d44b1
Summary: These NativeModules were easy to convert, since no other NativeModules in `React/Modules` depend on them.
Reviewed By: PeteTheHeat
Differential Revision: D16817959
fbshipit-source-id: 1036c2d437e1275776a185bf68c450c6454985df
Summary:
Due to an update to react-native on the android tv platform tapping the select button on a remote calls the onPress prop twice for `TouchableHighlight`, `TouchableOpacity`, and `TouchableWithoutFeedback`. This is happening because touchableHandlePress gets called from two places. First from the onClick prop in the touchable component and second from the TVEventHandler in the TouchableMixin.
## Changelog
[Android] [Fixed] - Adds a not android check to the select case of the TVEventHandler callback in the TouchableMixin.
Pull Request resolved: https://github.com/facebook/react-native/pull/26474
Test Plan:
Confirmed on Android Tv and Apple Tv
1) Add a TouchableOpacity to a screen with an onPress callback
2) Run app
3) Focus the TouchableOpacity
4) Press the Select Button on the Remote
**Expected Results**
onPress is called once
Differential Revision: D17530170
Pulled By: TheSavior
fbshipit-source-id: b776faba477c6231ad296abd21f072335dca5556
Summary:
When using a medium (500) font weight on Android the wrong weight is used for the placeholder and for the first few seconds of input (before it gets text back from JS). To fix it I refactored the way we handle text styles (family, weight, style) to create a typeface to be more like the `Text` component.
Since all these 3 props are linked and used to create the typeface object it makes more sense to do it at the end of setting props instead of in each prop handler and trying to recreate the object without losing styles set by other prop handlers. Do do that we now store fontFamily, fontStyle and fontWeight as ivar of the ReactEditText class. At the end of updating prop if any of those changed we recreate the typeface object.
This doesn't actually fix the bug but was a first step towards it. There were a bunch of TODOs in the code to remove duplication between `Text` and `TextInput` for parsing and creating the typeface object. To do that I simply moved the code to util functions in a static class. Once the duplication was removed the bug was fixed! I assume proper support for medium font weights was added for `Text` but not in the duplicated code for `TextInput`.
## Changelog
[Android] [Fixed] - Fix medium font weights for TextInput on Android
Pull Request resolved: https://github.com/facebook/react-native/pull/26434
Test Plan:
Tested in my app and in RNTester that custom styles for both text and textinput all seem to work.
Repro in RNTester:
```js
function Bug() {
const [value, setValue] = React.useState('');
return (
<TextInput
style={[
styles.singleLine,
{fontFamily: 'sans-serif', fontWeight: '500', fontSize: 32},
]}
placeholder="Sans-Serif 500"
value={value}
onChangeText={setValue}
/>
);
}
```
Before:
After:
Reviewed By: mmmulani
Differential Revision: D17468825
Pulled By: JoshuaGross
fbshipit-source-id: bc2219facb94668551a06a68b0ee4690e5474d40
Summary:
I happened to hit this error a couple times and the issue is that if there are let's say 1000 pending callbacks the error would be triggered 500 times and pretty much crash the app. I think it is reasonable to use warn once here so it only happens once.
## Changelog
[General] [Fixed] - Use `warnOnce` for excessive number of callbacks error
Pull Request resolved: https://github.com/facebook/react-native/pull/26508
Test Plan: Tested by reducing the number of pending callbacks required to trigger the error.
Reviewed By: TheSavior
Differential Revision: D17512917
Pulled By: JoshuaGross
fbshipit-source-id: 5ce8e2a0a166805cc6f3fe6d78e2716d6792a80e
Summary:
The previous version of the code accessed `_animIdIsManagedByFabric` on the main thread (which is should be accessed on the UIManager thread) and called `flushOperationQueues` on the main thread as well (also must be called on UIManager thread because it modifies instance variables (e.g. `_operations`) which supposed to be accessed on UIManager thread).
The diff fixes that introducing an additional queue jump. That's should be fine because the overall architecture of RCTNativeAnimatedModule is appeared to be asynchronous and should be resilient to possible races.
Reviewed By: sammy-SC
Differential Revision: D17523958
fbshipit-source-id: c4b4ce38b68b009726b2f6c28c38b32b9f9d6921
Summary: This diff migrates `ReactSwtichManager` to use the generated `ReactSwtichManagerDelegate` for setting its properties.
Reviewed By: TheSavior
Differential Revision: D17395067
fbshipit-source-id: 1489c5d08cef860030ecbd23ef19bd8de1328d71
Summary: This diff migrates `ReactDrawerLayoutManager` to use the generated `AndroidDrawerLayoutManagerDelegate` for setting its properties.
Reviewed By: mdvacca
Differential Revision: D17343383
fbshipit-source-id: 85cd7ee3531b152da2601048f5e458f5dad73ad6
Summary:
Some props must have their default values set by native. To be able to support this, we have to introduce a null as a supported default value for some types. In this diff I'm adding support for null default values for float props. An example of this to be useful is `ReactDrawerLayoutManager`:
```
Override
public void setDrawerWidth(ReactDrawerLayout view, Nullable Float width) {
int widthInPx =
width == null
? LayoutParams.MATCH_PARENT
: Math.round(PixelUtil.toPixelFromDIP(width));
view.setDrawerWidth(widthInPx);
}
```
We need to be able to generate an interface method, that accepts a boxed `Float` value instead of the primitive `float` so that the native code can decide what value to use by default (`LayoutParams.MATCH_PARENT` in this case).
Reviewed By: rickhanlonii
Differential Revision: D17343172
fbshipit-source-id: 7662a4e0e495f58d05a92892f063535a359d09ae
Summary: This diff migrates `ReactProgressBarViewManager` to use the generated `AndroidProgressBarManagerDelegate` for setting its properties.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D17315619
fbshipit-source-id: 6293c6fc18567a934b6f3dce9b77abcc408052d8
Summary: Some props must have their default values set by native. To be able to support this, we have to introduce a `null` as a supported default value for some types. In this diff I'm adding support for `null` default values for boolean props. Check D17260168 for the example of the usage of the nullable boolean values.
Reviewed By: rickhanlonii, TheSavior
Differential Revision: D17258234
fbshipit-source-id: 63b7864be97856704d5964230526f23c0e395a67
Summary: This diff migrates `SwipeRefreshLayoutManager` to use the generated `AndroidSwipeRefreshLayoutManagerDelegate`.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D17225894
fbshipit-source-id: e659d2a9cb5dba42c589559f61a0e98330e21612
Summary: This diff migrates `ReactModalHostManager` to use the generated `ModalHostViewManagerDelegate` for setting its properties.
Reviewed By: mdvacca
Differential Revision: D17205817
fbshipit-source-id: 6724302fd4301f9df92df04fcfb41f0c2c939d9f
Summary: This diff migrates `ReactSliderManager` to use the generated `SliderManagerDelegate` for setting its properties.
Reviewed By: mdvacca
Differential Revision: D17203078
fbshipit-source-id: 726736ef275074ecb799b334342ac64976153e2b
Summary: Small adjustment to order of operations. In case eventEmitter is nil, we still want to set image to nil.
Reviewed By: shergin
Differential Revision: D17503570
fbshipit-source-id: fd013c60e1188bcf63ff28ff7aad814582a3ae34
Summary: The OSS change in this diff allows `RCTDisplayLink` to call out to an observer, even if the `RCTModuleData` isn't well formed. This is the case in bridgeless RN.
Reviewed By: ejanzer
Differential Revision: D17438647
fbshipit-source-id: 00d4d61d9126902180a7a77fc702f4221cf4d779
Summary: The OSS file touched in this diff is just a styling change.
Reviewed By: ejanzer
Differential Revision: D17400387
fbshipit-source-id: e51884d8942ba01e7da1662ac24429070e53a504
Summary:
Similar to Android Venice impl, I plan to reuse old Timing native module because it's quite complex, and there's not much to gain from re-writing it. When we delete bridge access from it, we can remove any cruft, but IMO it's quite lean already.
The name of this class is a little confusing IMO, I originally had it because it was similar to `Timers.java` but then Emily renamed :p Here's the architecture drawn out:
{F209114665}
If anyone has a better name I'm all ears.
Reviewed By: RSNara
Differential Revision: D17376028
fbshipit-source-id: 066ae0b379cd00538021a1327a4dd63d5a828608
Summary:
Instead of relying on the bridge to do module setup, allow for regular initialization.
Instead of relying on the bridge to execute timers, allow for a delegate to do the work.
Reviewed By: RSNara
Differential Revision: D17375924
fbshipit-source-id: 83adabf8c962a5d90a4ea623618903cd9fb79a99
Summary: Fix for crash introduced by D17282188; doesn't cleanly revert, so just reverting this part
Reviewed By: JoshuaGross, RSNara
Differential Revision: D17505827
fbshipit-source-id: 3232285c0f15dabeb819f8806ad35d4ec83a1e53
Summary: Now that all the plumbing is done, this diff finally implements async method dispatch on the NativeModule thread.
Reviewed By: mdvacca
Differential Revision: D17480605
fbshipit-source-id: 992aab99954c488a0327144d84a1668a2b158d04
Summary:
Self explanatory.
1. **Existing:** We create the NativeModules thread in CatalystInstanceImpl.cpp.
2. D17422165: We wrap this thread in a `BridgeNativeCallInvoker`.
3. D17422164: We use `CatalystInstanceImpl::getNativeCallInvokerHolder()` to get a hold of the `BridgeNitiveCallInvoker` in Java.
4. D17422163: From Java, we pass this `CallInvokerHolder` to `TurboModuleManager`'s constructor.
5. **This diff:** `TurboModuleManager` then unwraps the `CallInvoker` from `CallInvokerHolder`, and passes it to all TurboModules in their constructor.
Reviewed By: PeteTheHeat
Differential Revision: D17422160
fbshipit-source-id: c0a76dfe5fdedac2e0e21f7a562bc7588dc190fb
Summary:
In the previous diffs, I:
1. D17422165: Created a `CallInvoker` for the NativeModules thread.
2. D17422164: Exposed this `CallInvoker` via `CatalystInstance.getNativeCallInvokerHolder()`.
In this diff, I:
1. Make TurboModuleManager accept the NativeModules thread `CallInvoker` as a constructor argument.
Reviewed By: PeteTheHeat
Differential Revision: D17422163
fbshipit-source-id: cbaac2fc06f7f726d89e0c545154123ad7410e62
Eli White
Logan Daniels
Uts Sikder
Joshua Gross
Mehdi Mulani
Emily Janzer
Valentin Shergin
Kevin Gozali
Luna Wei
Ramanpreet Nara
Daniel
Janic Duplessis
Oleksandr Padalko
Samuel Susla
Oleksandr Melnykov
Amir Shalem
Peter Argany
Mike Vitousek
