Summary:
Fragment was assigned incorrect `tag` and `surfaceID` (`surfaceID` is the important one).
Wrong `surfaceID` means that `navigationCoordinator` is never resolved. As a result of navigationCoordinator not being assigned, tapping a video ad on Marketplace results in showing video ad overlay rather than showing full screen video.
Reviewed By: JoshuaGross
Differential Revision: D16646492
fbshipit-source-id: 0da5c56ecb7c81e9f4a9469a3626ccd430a01558
Summary:
## The Problem
1. `CatalystInstanceImpl` indirectly holds on to the `jsi::Runtime`. When you destroy `CatalystInstanceImpl`, you destroy the `jsi::Runtime`. As a part of reloading React Native, we destroy and re-create `CatalystInstanceImpl`, which destroys and re-creates the `jsi::Runtime`.
2. When JS passes in a callback to a TurboModule method, we take that callback (a `jsi::Function`) and wrap it in a Java `Callback` (implemented by `JCxxCallbackImpl`). This Java `Callback`, when executed, schedules the `jsi::Function` to be invoked on a Java thread at a later point in time. **Note:** The Java NativeModule can hold on to the Java `Callback` (and, by transitivity, the `jsi::Function`) for potentially forever.
3. It is a requirement of `jsi::Runtime` that all objects associated with the Runtime (ex: `jsi::Function`) must be destroyed before the Runtime itself is destroyed. See: https://fburl.com/m3mqk6wt
### jsi.h
```
/// .................................................... In addition, to
/// make shutdown safe, destruction of objects associated with the Runtime
/// must be destroyed before the Runtime is destroyed, or from the
/// destructor of a managed HostObject or HostFunction. Informally, this
/// means that the main source of unsafe behavior is to hold a jsi object
/// in a non-Runtime-managed object, and not clean it up before the Runtime
/// is shut down. If your lifecycle is such that avoiding this is hard,
/// you will probably need to do use your own locks.
class Runtime {
public:
virtual ~Runtime();
```
Therefore, when you delete `CatalystInstanceImpl`, you could end up with a situation where the `jsi::Runtime` is destroyed before all `jsi::Function`s are destroyed. In dev, this leads the program to crash when you reload the app after having used a TurboModule method that uses callbacks.
## The Solution
If the only reference to a `HostObject` or a `HostFunction` is in the JS Heap, then the `HostObject` and `HostFunction` destructors can destroy JSI objects. The TurboModule cache is the only thing, aside from the JS Heap, that holds a reference to all C++ TurboModules. But that cache (and the entire native side of `TurboModuleManager`) is destroyed when we call `mHybridData.resetNative()` in `TurboModuleManager.onCatalystInstanceDestroy()` in D16552730. (I verified this by commenting out `mHybridData.resetNative()` and placing a breakpoint in the destructor of `JavaTurboModule`). So, when we're cleaning up `TurboModuleManager`, the only reference to a Java TurboModule is the JS Heap. Therefore, it's safe and correct for us to destroy all `jsi::Function`s created by the Java TurboModule in `~JavaTurboModule`. So, in this diff, I keep a set of all `CallbackWrappers`, and explicitly call `destroy()` on them in the `JavaTurboModule` destructor. Note that since `~JavaTurboModule` accesses `callbackWrappers_`, it must be executed on the JS Thread, since `createJavaCallbackFromJSIFunction` also accesses `callbackWrappers_` on the JS Thread.
For additional safety, I also eagerly destroyed the `jsi::Function` after it's been invoked once. I'm not yet sure if we only want JS callbacks to only ever be invoked once. So, I've created a Task to document this work: T48128233.
Reviewed By: mdvacca
Differential Revision: D16623340
fbshipit-source-id: 3a4c3efc70b9b3c8d329f19fdf4b4423c489695b
Summary: This diff implements the JSResponderHandler methods in the core of RN (scheduler API and friends)
Reviewed By: ejanzer
Differential Revision: D16543437
fbshipit-source-id: dac03e30c4330d182ecf134f3174ba942dbf7289
Summary: When you create a TurboModule from the JS side, we instantiate its Java class and simply make this `javaobject` a `jni::global_ref` in C++. But the reason why we need to make this a global ref is because `JavaTurboModule` needs it to be a global reference for method calls. Making this a `jni::global_ref` from the perspective to TurboModuleManager doesn't really make any sense. So, this diff refactors that bit of code.
Differential Revision: D16622133
fbshipit-source-id: 6a5c20bb405b945c06378a3423d5e7eb38ef244c
Summary: On iOS, calling the `__turboModuleProxy` function with the same name returns the same instance of the TurboModule. Adding this behaviour to Andorid as well.
Differential Revision: D16622131
fbshipit-source-id: 472011ac3356e7c30497f848be0c888596c449b1
Summary: Fabric ObjC(++) files will be prefixed by RN* for the time being, this codemod is a simple rename. This includes `interface` and `protocol` definition
Reviewed By: PeteTheHeat, yungsters
Differential Revision: D16611524
fbshipit-source-id: 868d2571ea2414dde4cbb3b75b1334b779b5d832
Summary:
## The Problem
1. `CatalystInstanceImpl` indirectly holds on to the `jsi::Runtime`. When you destroy `CatalystInstanceImpl`, you destroy the `jsi::Runtime`. As a part of reloading React Native, we destroy and re-create `CatalystInstanceImpl`, which destroys and re-creates the `jsi::Runtime`.
2. When JS passes in a callback to a TurboModule method, we take that callback (a `jsi::Function`) and wrap it in a Java `Callback` (implemented by `JCxxCallbackImpl`). This Java `Callback`, when executed, schedules the `jsi::Function` to be invoked on a Java thread at a later point in time. **Note:** The Java NativeModule can hold on to the Java `Callback` (and, by transitivity, the `jsi::Function`) for potentially forever.
3. It is a requirement of `jsi::Runtime` that all objects associated with the Runtime (ex: `jsi::Function`) must be destroyed before the Runtime itself is destroyed. See: https://fburl.com/m3mqk6wt
### jsi.h
```
/// .................................................... In addition, to
/// make shutdown safe, destruction of objects associated with the Runtime
/// must be destroyed before the Runtime is destroyed, or from the
/// destructor of a managed HostObject or HostFunction. Informally, this
/// means that the main source of unsafe behavior is to hold a jsi object
/// in a non-Runtime-managed object, and not clean it up before the Runtime
/// is shut down. If your lifecycle is such that avoiding this is hard,
/// you will probably need to do use your own locks.
class Runtime {
public:
virtual ~Runtime();
```
Therefore, when you delete `CatalystInstanceImpl`, you could end up with a situation where the `jsi::Runtime` is destroyed before all `jsi::Function`s are destroyed. In dev, this leads the program to crash when you reload the app after having used a TurboModule method that uses callbacks.
## The Solution
If the only reference to a `HostObject` or a `HostFunction` is in the JS Heap, then the `HostObject` and `HostFunction` destructors can destroy JSI objects. The TurboModule cache is the only thing, aside from the JS Heap, that holds a reference to all C++ TurboModules. But that cache (and the entire native side of `TurboModuleManager`) is destroyed when we call `mHybridData.resetNative()` in `TurboModuleManager.onCatalystInstanceDestroy()` in D16552730. (I verified this by commenting out `mHybridData.resetNative()` and placing a breakpoint in the destructor of `JavaTurboModule`). So, when we're cleaning up `TurboModuleManager`, the only reference to a Java TurboModule is the JS Heap. Therefore, it's safe and correct for us to destroy all `jsi::Function`s created by the Java TurboModule in `~JavaTurboModule`. So, in this diff, I keep a set of all `CallbackWrappers`, and explicitly call `destroy()` on them in the `JavaTurboModule` destructor. Note that since `~JavaTurboModule` accesses `callbackWrappers_`, it must be executed on the JS Thread, since `createJavaCallbackFromJSIFunction` also accesses `callbackWrappers_` on the JS Thread.
For additional safety, I also eagerly destroyed the `jsi::Function` after it's been invoked once. I'm not yet sure if we only want JS callbacks to only ever be invoked once. So, I've created a Task to document this work: T48128233.
Reviewed By: mhorowitz
Differential Revision: D16589168
fbshipit-source-id: a1c0786999c22bef55d416beb0fc40261447a807
Summary: When you create a TurboModule from the JS side, we instantiate its Java class and simply make this `javaobject` a `jni::global_ref` in C++. But the reason why we need to make this a global ref is because `JavaTurboModule` needs it to be a global reference for method calls. Making this a `jni::global_ref` from the perspective to TurboModuleManager doesn't really make any sense. So, this diff refactors that bit of code.
Reviewed By: mdvacca
Differential Revision: D16555673
fbshipit-source-id: 2778fc5a372c41847e8296c2e22bb9a8826fcc52
Summary: On iOS, calling the `__turboModuleProxy` function with the same name returns the same instance of the TurboModule. Adding this behaviour to Andorid as well.
Reviewed By: mdvacca
Differential Revision: D16553363
fbshipit-source-id: c95e150d6967604a808cfb49877b7a633e33d729
Summary:
Added an array to maintain the counts of each of the reason of measure callbacks
and this is now added as qpl metadata in Layout Calculation qpl event
Reviewed By: davidaurelio
Differential Revision: D16516379
fbshipit-source-id: 201c5d2463f0a921841a0bbfec8f4d5e007000c8
Summary:
We had flex as a reason for both layout and measure. Now creating separating reason flexLayout and flexMeasure in this diff.
Also changed ordering of items in Enum to group layout and measure reasons
Reviewed By: davidaurelio
Differential Revision: D16562350
fbshipit-source-id: 75501f9d4dde0974009193b3991a8acc97b02ad0
Summary:
This diff changes the name of the DispatchCommand methods in the UIManagerDelegate and Scheduler classes.
The purpose of this change is to use a consistent naming with the rest of the methods od these classes.
We might re-name these interfaces later, but for now we want to keep a consistent naming. For more details see discussion in D16543437
Reviewed By: JoshuaGross
Differential Revision: D16575403
fbshipit-source-id: 8335be8dc3367372a8ef2d7e8ed78665f4c02699
Summary: I think it's possible that there's a race condition between creating the scheduler and setting the delegate leading to bugs like T47272192.
Reviewed By: mdvacca
Differential Revision: D16537737
fbshipit-source-id: 9c579537658be5a9aeed37c0e4935c997cabb6aa
Summary:
# Problem:
I enabled Fabric for "Profile About", when I navigate to "Marketplace Home" from "Profile About", I get following warning "<Component> is not Fabric compatible yet". Even though the component has been migrated to Fabric and is getting registered. The component works if I navigate straight to "Marketplace Home".
{F172219008}
# How it was solved:
It turns out, there was a missing call that added newly created registry to `componentDescriptorRegistries_`.
Reviewed By: shergin
Differential Revision: D16542109
fbshipit-source-id: 22a315333cfff0895ce77e8439816f4e7bf34d35
Summary:
Yesterday we shipped hermesengine.dev as part of the current 0.60 release. This PR brings those changes to master.
## Changelog
[General] [Added] - Added support for Hermes
Pull Request resolved: https://github.com/facebook/react-native/pull/25613
Test Plan:
* CI is green both on GitHub and at FB
* Creating a new app from source can use Hermes on Android
Reviewed By: cpojer
Differential Revision: D16221777
Pulled By: willholen
fbshipit-source-id: aa6be10537863039cb666292465ba2e1d44b64ef
Summary:
As part of the fix for https://github.com/facebook/react-native/issues/25349 I added `s.static_framework = true` to each podspec in repo (see https://github.com/facebook/react-native/pull/25619#discussion_r306993309 for more context).
This was required to ensure the existing conditional compilation with `#if RCT_DEV` and `__has_include` still worked correctly when `use_frameworks!` is enabled.
However, fkgozali pointed out that it would be ideal if we didn't have this requirement as it could make life difficult for third-party libraries.
This removes the requirement by moving `React-DevSupport.podspec` and `React-RCTWebSocket.podspec` into `React-Core.podspec` as subspecs. This means the symbols are present when `React-Core.podspec` is built dynamically so `s.static_framework = true` isn't required.
This means that any `Podfile` that refers to `React-DevSupport` or `React-RCTWebSocket` will need to be updated to avoid errors.
## Changelog
I don't think this needs a changelog entry since its just a refinement of https://github.com/facebook/react-native/pull/25619.
Pull Request resolved: https://github.com/facebook/react-native/pull/25816
Test Plan:
Check `RNTesterPods` still works both with and without `use_frameworks!`:
1. Go to the `RNTester` directory and run `pod install`.
2. Run the tests in `RNTesterPods.xcworkspace` to see that everything still works fine.
3. Uncomment the `use_frameworks!` line at the top of `RNTester/Podfile` and run `pod install` again.
4. Run the tests again and see that it still works with frameworks enabled.
Reviewed By: hramos
Differential Revision: D16495030
Pulled By: fkgozali
fbshipit-source-id: 2708ac9fd20cd04cb0aea61b2e8ab0d931dfb6d5
Summary:
Implements a new host function on the global object in debug builds, called `globalEvalWithSourceUrl`. This performs a global `eval()` and attaches a URL/filename to the evaluated script (in stack traces, debuggers, etc).
It serves a similar purpose to the `//# sourceURL=` directive (which most JS engines support, but JSC doesn't) and to the old `nativeInjectHMRUpdate` function which was dropped in the JSC->JSI migration.
Reviewed By: cpojer
Differential Revision: D16491506
fbshipit-source-id: bd9a89311dcbb1d0baece77ead16b9ecfb13bfe3
Summary:
This is my proposal for fixing `use_frameworks!` compatibility without breaking all `<React/*>` imports I outlined in https://github.com/facebook/react-native/pull/25393#issuecomment-508457700. If accepted, it will fix https://github.com/facebook/react-native/issues/25349.
It builds on the changes I made in https://github.com/facebook/react-native/pull/25496 by ensuring each podspec has a unique value for `header_dir` so that framework imports do not conflict. Every podspec which should be included in the `<React/*>` namespace now includes it's headers from `React-Core.podspec`.
The following pods can still be imported with `<React/*>` and so should not have breaking changes: `React-ART`,`React-DevSupport`, `React-CoreModules`, `React-RCTActionSheet`, `React-RCTAnimation`, `React-RCTBlob`, `React-RCTImage`, `React-RCTLinking`, `React-RCTNetwork`, `React-RCTPushNotification`, `React-RCTSettings`, `React-RCTText`, `React-RCTSettings`, `React-RCTVibration`, `React-RCTWebSocket` .
There are still a few breaking changes which I hope will be acceptable:
- `React-Core.podspec` has been moved to the root of the project. Any `Podfile` that references it will need to update the path.
- ~~`React-turbomodule-core`'s headers now live under `<turbomodule/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511091823.
- ~~`React-turbomodulesamples`'s headers now live under `<turbomodulesamples/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511091823.
- ~~`React-TypeSaferty`'s headers now live under `<TypeSafety/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511040967.
- ~~`React-jscallinvoker`'s headers now live under `<jscallinvoker/*>`~~ Replaced by https://github.com/facebook/react-native/pull/25619#issuecomment-511091823.
- Each podspec now uses `s.static_framework = true`. This means that a minimum of CocoaPods 1.5 ([released in April 2018](http://blog.cocoapods.org/CocoaPods-1.5.0/)) is now required. This is needed so that the ` __has_include` conditions can still work when frameworks are enabled.
Still to do:
- ~~Including `React-turbomodule-core` with `use_frameworks!` enabled causes the C++ import failures we saw in https://github.com/facebook/react-native/issues/25349. I'm sure it will be possible to fix this but I need to dig deeper (perhaps a custom modulemap would be needed).~~ Addressed by 33573511f0.
- I haven't got Fabric working yet. I wonder if it would be acceptable to move Fabric out of the `<React/*>` namespace since it is new? �
## Changelog
[iOS] [Fixed] - Fixed compatibility with CocoaPods frameworks.
Pull Request resolved: https://github.com/facebook/react-native/pull/25619
Test Plan:
### FB
```
buck build catalyst
```
### Sample Project
Everything should work exactly as before, where `use_frameworks!` is not in `Podfile`s. I have a branch on my [sample project](https://github.com/jtreanor/react-native-cocoapods-frameworks) here which has `use_frameworks!` in its `Podfile` to demonstrate this is fixed.
You can see that it works with these steps:
1. `git clone git@github.com:jtreanor/react-native-cocoapods-frameworks.git`
2. `git checkout fix-frameworks-subspecs`
3. `cd ios && pod install`
4. `cd .. && react-native run-ios`
The sample app will build and run successfully. To see that it still works without frameworks, remove `use_frameworks!` from the `Podfile` and do steps 3 and 4 again.
### RNTesterPods
`RNTesterPodsPods` can now work with or without `use_frameworks!`.
1. Go to the `RNTester` directory and run `pod install`.
2. Run the tests in `RNTesterPods.xcworkspace` to see that everything still works fine.
3. Uncomment the `use_frameworks!` line at the top of `RNTester/Podfile` and run `pod install` again.
4. Run the tests again and see that it still works with frameworks enabled.
Reviewed By: PeteTheHeat
Differential Revision: D16465247
Pulled By: PeteTheHeat
fbshipit-source-id: cad837e9cced06d30cc5b372af1c65c7780b9e7a
Summary: Right now RuntimeExecutor is only used in Fabric. Moving it out of Fabric's uimanager/primitives.h and into react/utils so we can use it more broadly.
Reviewed By: shergin
Differential Revision: D16385366
fbshipit-source-id: 96063e536e1480bac078a9376fe55f7d8750477e
Summary: This diff exposes the legacy measure() method on Fabric. This method will be replaced by getRelativeLayoutMetrics in the future, but we are exposing this in order to make Fabric migration easier.
Reviewed By: JoshuaGross, shergin
Differential Revision: D16432928
fbshipit-source-id: 25cc21fb48ec0749081f112af9230bfe53314d6b
Summary: This diff exposes the new methods SetJSResponder and clearJSResponder in the UI ManagerBinding interface
Reviewed By: shergin
Differential Revision: D16420689
fbshipit-source-id: 606bede1de6b9d5fd5a56e832ad27100b6998c55
Summary:
This change lets `registerBundle(bundleId, file)` throw an exception
when the file is empty, improving on the current behavior of an
eventual SIGABRT saying "MAP_FAILED: Invalid argument"
Reviewed By: ridiculousfish
Differential Revision: D16451938
fbshipit-source-id: b8b2d0bfed476319c379122fad59a5bf0a8c813b
Summary: Been reading a lot of code comments getting familiar with Fabric & TM, just fixing a few typos and removing an unused bridge category method.
Reviewed By: shergin
Differential Revision: D16371581
fbshipit-source-id: bf0cc9c873c60e37124dc715c92d7f105e54e42f
Summary: Supporting View Manager Commands on the new UIManager in Fabric. This is needed for things like scrollTo on ScrollView.
Reviewed By: JoshuaGross
Differential Revision: D16175575
fbshipit-source-id: a74effdf7e47b56a150a4e3fb6c4d787659e0250
Summary: Adds internal API that we can use to conduct experiments.
Reviewed By: SidharthGuglani
Differential Revision: D16340463
fbshipit-source-id: 07a8bb7dbc4a02c5c95f1ad29b18845ab43752cf
Summary: Right now we register ReactFabric as a callable module with the bridge so that we can call `ReactFabric.unmountComponentAtNode` in `ReactInstanceManager.detachViewFromInstance`. In bridgeless mode we don't have callable modules, so I'm just setting a global variable that can be called from C++ instead. Using this in a new `unmount` method in FabricUIManager.
Reviewed By: shergin, mdvacca
Differential Revision: D16273720
fbshipit-source-id: 95edb16da6566113a58babda3ebdf0fc4e39f8b0
Summary:
Remove check whether `measureCache_` is nullptr. It was part of the experiment
which is no longer running.
Reviewed By: shergin
Differential Revision: D16360332
fbshipit-source-id: 2fc8baa93a87754da6255bf1c134901447349f7a
Summary: Using enum struct for using enums in form ENUM_NAME::ENUM_VALUE for better code readablility
Reviewed By: davidaurelio
Differential Revision: D16356562
fbshipit-source-id: cbe7adadad78eb5d0756c44679c0e102b7d31ec6
Summary:
`YGStyle` puts Yoga enums (which are signed integers by default) into bitfields: https://fburl.com/7fowlunu
Mixing signed values and bit-fields can be error-prone and it also fails to build on Windows with `clang-cl` due to `-Wbitfield-constant-conversion` warning being treated as error:
```
stderr: In file included from xplat\yoga\yoga\YGLayout.cpp:8:
In file included from xplat\yoga\yoga/Utils.h:8:
In file included from xplat\yoga\yoga/YGNode.h:13:
xplat\yoga\yoga/YGStyle.h(110,9): error: implicit truncation from 'YGAlign' to bit-field changes value from 4 to -4 [-Werror,-Wbitfield-constant-conversion]
alignItems_(YGAlignStretch),
```
This diff fixes the problem by making all enums unsigned integers. This change can be problematic only if values of the enums are serialized somewhere. CC: David Aurelio
Reviewed By: davidaurelio
Differential Revision: D16336729
fbshipit-source-id: ee4dabd7bd1ee429e644bd322b375ec2694cc742
Summary:
Based on Hermes Issue: https://github.com/facebook/hermes/issues/47
It was not actually a bug in Hermes, but a bug in JSI, assuming that all string property names from
`folly::dynamic` are ASCII.
Now we'll be more intentional and directly state `forUtf8` rather than the implicit ASCII encoding.
Reviewed By: mhorowitz
Differential Revision: D16347857
fbshipit-source-id: 6bcfbf9f918dc0a7a485b88a1b537d6c2dd322cc
Summary: Ran clang-format on all of JSI.
Reviewed By: mhorowitz
Differential Revision: D16347858
fbshipit-source-id: 004afde1944f60a2c0989a7c38d5b3c58587f1cb
Summary:
`stubViewTreeFromShadowNode` was implemented without using `calculateShadowViewMutations` (aka Diffing algorithm).
The problem is that we use `stubViewTreeFromShadowNode` to test the diffing, so it was not fully correct to use the algorithm to test itself.
Reviewed By: JoshuaGross
Differential Revision: D16342526
fbshipit-source-id: 2674245ed5ec71309fe5d362779a33d8dd31dc7b
Summary: That's extremly helpful for debugging, it allows to see the generated view structure (as diffing sees it).
Reviewed By: JoshuaGross
Differential Revision: D16342528
fbshipit-source-id: bc303d5cab992e517b7cf29962d7c1232e8aeec7
Summary:
1. The check is now correct.
2. Now we call that "Invalid", not "Empty" because this case is invalid indeed.
Reviewed By: JoshuaGross
Differential Revision: D16342527
fbshipit-source-id: 433f6c5759d2d0756d55acb057b5639a8eab2e5d
Summary:
Trivial.
Before that we didn't have a way to get a root node from a Tree object.
It's useful during debugging (and some coming changes use that).
Reviewed By: mdvacca
Differential Revision: D16342529
fbshipit-source-id: 27c7516f3b3fccc8d8b25d4d2748006f7958ae41
Summary:
This diff fixes the parsing of the textAlign prop in Fabric. The current parsing does not support 'justify' or 'auto' values.
See https://facebook.github.io/react-native/docs/0.59/text-style-props#textalign for more details about the values of these props in the JS side.
Reviewed By: shergin
Differential Revision: D16269509
fbshipit-source-id: e0d9168d6022245430de644f7c4e45c968b1326b
Summary:
When you call a TurboModule method with JS arguments, we necessarily convert these JS args to Java objects/primitives before passing them to the Java implementation of the method. The problem is that we let the type of the JS arg dictate the type of the Java object/primitive to convert said arg to. This means that if a JS developer passes in an `number` to a function that expects an `Array`, we'll convert the number to a `double` and try to call the Java method with that `double`, when it actually expects a `ReadableArray`. This will trigger a JNI error, and crash the program. Ideally, we should be able to catch these type mismatches early on.
In this diff, on every TurboModule method call, I parse the method signature to determine the Java type of each JS argument. Then, for any argument, if the JS arg and the Java arg types aren't compatible, I raise an exception, which gets displayed as a RedBox in development. This diff also implements support for `?number` and `?boolean` argument and return types in TurboModules.
Reviewed By: mdvacca
Differential Revision: D16214814
fbshipit-source-id: 4399bb88c5344cff50aa8fe8d54eb2000990a852
Summary:
This is the first step towards fixing https://github.com/facebook/react-native/issues/25349. These are the changes to the podspec to correctly update dependencies and build config that will cause any breaking change for users or libraries.
I am breaking these changes out from https://github.com/facebook/react-native/pull/25393 as suggested by fkgozali in https://github.com/facebook/react-native/pull/25393#issuecomment-508322884.
These are the changes:
- Made C++ headers in `React-Core` private by default so that ObjC files can import the module without failures.
- Reduced the number of `yoga` headers that are exposed for the same reason as above. As far as I can see this doesn't cause issues but we can find another solution if it does.
- Adding some missing dependencies to fix undefined symbols errors.
- Added `DoubleConversion` to `HEADER_SEARCH_PATHS` where it was missing.
## Changelog
[iOS] [Fixed] - Updated podspecs for improved compatibility with different install types.
Pull Request resolved: https://github.com/facebook/react-native/pull/25496
Test Plan:
Everything should work exactly as before. I have a branch on my [sample project](https://github.com/jtreanor/react-native-cocoapods-frameworks) here which points at this branch to show that it is still working `Podfile` to demonstrate this is fixed.
You can see that it works with these steps:
1. `git clone git@github.com:jtreanor/react-native-cocoapods-frameworks.git`
2. `git checkout podspec-updates`
3. `cd ios && pod install`
4. `cd .. && react-native run-ios`
The sample app will build and run successfully.
Reviewed By: mmmulani
Differential Revision: D16167346
Pulled By: fkgozali
fbshipit-source-id: 1917b2f8779cb172362a457fb3fce686c55056d3
Summary: That should help to fail early in situations when we lose critical class data members.
Reviewed By: sammy-SC
Differential Revision: D16179539
fbshipit-source-id: da73b81568c2f3657b9bc2bd1cc7ee6624e75626
Summary: This is a very similar mechanism that we use in UIManager and it should be eventually unified.
Reviewed By: sammy-SC
Differential Revision: D16179524
fbshipit-source-id: 7c8c45b7581ac4a1db3a773d62004ff368f18321
Summary: Adds Baseline start and end events to be handled later for instrumentation
Reviewed By: davidaurelio
Differential Revision: D16048790
fbshipit-source-id: 8409dbb633168753a7bf8fab20bc6551d113ddd6
Summary: Using layoutPassStart and LayoutPassEnd events instead of YGMarkerLayout for instrumentation
Reviewed By: davidaurelio
Differential Revision: D16048789
fbshipit-source-id: 041a35bc2cb1b7281ca83cf9d35041b4011cfeb9
Summary: Accessibility is essential but seems we don't have any use of AccessibleShadowNode in Fabric.
Reviewed By: sammy-SC
Differential Revision: D16139595
fbshipit-source-id: a6aec6d22c4a6050d7ee5e6b21ef4ad04d80ffce
Summary:
The instrumentation header only needs the forward declarations for ostream, so
we can use just include `iosfwd`, as suggested by Riley in an earlier diff.
Reviewed By: kodafb
Differential Revision: D16152451
fbshipit-source-id: 2afbc40e623b180dfc5917fc8093ab15bf647968
Summary:
All props to Eric Lewis! cc ericlewis
This revert of revert of land of changes originally published in #24873 (with some slight fixes). The change removes usage of LocalData from the `<Text>` component.
After this change we only have ---one (maybe two)--- three components left using LocalData.
Reviewed By: mdvacca
Differential Revision: D15962376
fbshipit-source-id: 19f41109ce9d71ce30d618a45eb2b547a11f29a2
Summary: This diff exposes LayoutDirection as part of UpdateLayoutMountItem
Reviewed By: JoshuaGross
Differential Revision: D16060521
fbshipit-source-id: 163bf2a0bdca62dcecb03a8aaa2f4bf595b18c8f
Summary: This fixes the parsing of textAlign prop in BaseTextProps class. The name is textAlign and not alignment.
Reviewed By: JoshuaGross
Differential Revision: D16057477
fbshipit-source-id: a00a472af4c7df0a6cd3d6efb17b30cd9134907b
Summary:
Previously, we stored `keyIndex_` in a Parser object which is incorrect because it makes the parsing process thread-unsafe (the Parser is shared between parsing processes).
That worked previously most of the time because we never parsed props concurrently but we actually do this in the native animation library.
Reviewed By: yungsters
Differential Revision: D16064557
fbshipit-source-id: 211f4301d0746e0f5126a1dcdd59f1ae9a420aa9
Summary:
Instead of checking whether `YG_ENABLE_EVENTS` is defined for every publish, we simply wrap the body of the `publish` function macro that delegates to the method that actually publishes the event.
This way we get
1. easier to write code where we publish events
2. more type safety when editing, enabling editors/IDEs to show errors without knowing about `YG_ENABLE_EVENTS`
Reviewed By: SidharthGuglani
Differential Revision: D16049888
fbshipit-source-id: cbf362d6f7be5053c3f377125d303b7137d6a241
Summary: Removes time measurements for measure callbacks. This functionality should not be part of Yoga/core, and can now be done by event subscribers themselves, as we have two events per measure callback.
Reviewed By: SidharthGuglani
Differential Revision: D16049812
fbshipit-source-id: e16556f3854e42f4bada39a97a668e718719b22c
Summary: Publishing two events will allow us to replace marker functionality completely with events. This also allows us to remove measuring time spent from Yoga itself.
Reviewed By: SidharthGuglani
Differential Revision: D16049810
fbshipit-source-id: 98628a92ed3c94d479e9fbcd53fac90c5f524087
Summary:
Adding an instrumentation endpoint that allows us to write a snapshot out to an
arbitrary output stream. This is in addition to `createSnapshotFromFile`. I
reserve the right to replace the latter with the former in a later diff.
This is necessary to allow snapshots to be requested over the chrome debugger
protocol. The protocol sends the snapshot over the wire in chunks so we need
to be able to use an output stream that can do the chunking.
Because LLVM types are not available at the JSI layer, we accept a
`std::ostream` reference which we wrap with `llvm::raw_os_ostream` within
Hermes. We should not incur the static initializer cost (or other code bloat)
of using ostreams as a result.
Reviewed By: cwdick
Differential Revision: D16016319
fbshipit-source-id: 2d0b4848fd5cbe9ddee371d856cd8eb19dd80396
Summary:
@public
Removes the declaration of `YGRoundValueToPixelGrid` from `Yoga-internal.h`, as it is already declared in `Yoga.h`. `Yoga.h` is included from `Yoga-internal.h`
Reviewed By: SidharthGuglani
Differential Revision: D16047832
fbshipit-source-id: 72d9d2510372c983eedacc5d7af406b9346f18e6
Summary:
The function is not annotated with `const` so `plain()` will return a non-const
reference to the undecorated Runtime already. Seems like the const_cast was a
hold-over from a previous iteration.
Reviewed By: mhorowitz
Differential Revision: D16016320
fbshipit-source-id: 3dfa1e9acf2fc5c1ad61c9a8cd27c3c2e42036d3
Summary: In some cases, we cannot retrieve the "committed" state because no one state was mounted yet. The whole concept of "confirmed" or "legit at the moment" is kinda overstatement. The actual meaning of this is "the last vision of the state to which we advanced so far"; it does not have any relation to the actual "commit" phase or "mounting" process.
Reviewed By: sammy-SC
Differential Revision: D16002127
fbshipit-source-id: 95465e632525f873ae67f6db320a89562b62ba29
Summary:
Continuing https://github.com/facebook/yoga/pull/791
nokia6686 is a former member of our team, so we are trying to pick up what he left and carry out the pull request.
# Solution
Improved from previous solution with jpap's suggestions.
2. Passing ```gDepth``` and ```gCurrentGenerationCount``` (renamed to **_depth_** and **_generationCount_** respectively) between function calls that stem from ```YGNodeCalculateLayout```.
In ```YGNodeCalculateLayout```, pass ```depth``` as value 0, to indicate the root depth.
Pull Request resolved: https://github.com/facebook/yoga/pull/852
Reviewed By: SidharthGuglani
Differential Revision: D15537450
Pulled By: davidaurelio
fbshipit-source-id: 338f51383591ba27702ebe759f6c47c2dede3530
Summary: `Target` inside the Stage can be empty; in this case, we should not try to extract `ShadowNode` from it.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D15982479
fbshipit-source-id: 83a4bebadc88b59d7fe77acbdf07e8ce9f2f6be1
Summary: Passing whether layout cache or measure cache was used or not
Reviewed By: davidaurelio
Differential Revision: D15920937
fbshipit-source-id: a6728e7af07ea228a285f824fbdfddc8130c5990
Summary:
Added event NodeLayoutEnd and this is being used now instead of NodeLayout
It will be used later to add more information about caches
Reviewed By: davidaurelio
Differential Revision: D15920935
fbshipit-source-id: c9f5e193bc8cc70d26ff5d84882d483c9b09f67d
Summary:
The previous version of a set of default values of `ShadowView`'s fields has a bug:
```
ComponentName componentName = "";
```
Initalizing `char const *` with a string literal in .h file makes the default constructor of the object produces different values across binary units (because a pointer to empty string can be defined differently). Now it's just a null pointer.
Reviewed By: sammy-SC
Differential Revision: D15911452
fbshipit-source-id: 16bcfb5f78ea802c0833135c486e3fbb8b7acaa6
Summary: I am about to change the definition of ShadowView a bit, so I think we need to ensure that we don't regress move semantic.
Reviewed By: JoshuaGross, sammy-SC
Differential Revision: D15911453
fbshipit-source-id: ce2cd4cb2375ecb76295948a1e9b5d2e7fb80f38
Summary: Returning a shared pointer by const reference in this context is not correct/safe because the object (the ShadowNode) doesn't own the object, so the caller cannot reason about the lifetime (esp. in a multithreaded environment).
Reviewed By: mdvacca
Differential Revision: D15958737
fbshipit-source-id: 8f03e6530d07d63ece5f955055c5c67c204b8223
Summary: There are still race condition during bridge invalidation. Some modules may be accessing other modules during invalidation, but it's racing with the TM manager clearing the cache.
Reviewed By: JoshuaGross
Differential Revision: D15947488
fbshipit-source-id: 3bd51382264f538a03ca565b0f099da40c3daadf
Summary:
Backward compatibility for `invalidate` method, because for the old module system, we ensure call all methods of modules on `methodQueue`, we also need to keep this rule to avoid some race condition in module.
## Changelog
[iOS] [Fixed] - Backward compatibility of calling invalidate on module's method queue
Pull Request resolved: https://github.com/facebook/react-native/pull/25264
Test Plan: Ensure `invalidate` method of `module` be called on `methodQueue`.
Reviewed By: PeteTheHeat, JoshuaGross
Differential Revision: D15944532
Pulled By: fkgozali
fbshipit-source-id: e260792bc6b86a48cdf376282063cbabccbf26f0
Summary: Replaces the relative include to `YGEnums.h` in `yoga/event/event.h` with `#include <yoga/YGEnums.h>
Reviewed By: SidharthGuglani
Differential Revision: D15778634
fbshipit-source-id: 2bceeb58f26c0d9d0df6c0e7ea20b8ddf68a1ee5
Summary:
It's unclear why, but the introspection infra is enabled somehow in prod.
It must be some kind of misconfiguration somewhere. Until we figure it now, we have to disable it completly.
Reviewed By: mdvacca
Differential Revision: D15908765
fbshipit-source-id: 2b17c61938731d43eaef51b070a4ec1ae9e5e1df
Summary:
ComponentName is used by many core component of React Native, such as ComponentDescriptor, ShadowNode, ShadowView and so on. In all those cases this value represents the actual name of the component which came from `concreteComponentName` template parameter of ConcreteShadowNode. In all of those cases, it's raw `char const *` type. So, we don't need to use owning representation of the string (std::string) in all those places.
The only exception from this is a part where we receive the name of the component from JS side. In this case, the source string comes from JS and has to be analyzed as a character sequence to find corresponding ComponentDescriptor.
In my experiments, 20% of the time during diffing is spent on copying (this) `std::string`.
Reviewed By: mdvacca
Differential Revision: D15844407
fbshipit-source-id: a2e71505e22d09107e001bdf661d4a826bcf2dea
Summary: Seems RCTImageLoader is not thread-safe, so we need to compensate for this for now. Classic RN mostly accesses the loader from the main thread (non-concurrently), so it mostly works for Paper.
Reviewed By: mdvacca
Differential Revision: D15867574
fbshipit-source-id: 4aad5570b57a136aa0bbe31d65f1afe2ae6e380e
Summary:
Adds the ability to `MarkerSection` to end the marker before it goes out of scope.
This unlocks two scenarios:
- reuse the data associated with a marker after ending it.
- end markers in the middle of a function without adding arbitrary blocks.
Reviewed By: SidharthGuglani
Differential Revision: D15837840
fbshipit-source-id: c0afaeeabd169c65189b5028be54ea7dac3e3b84
Summary: Counts how many times measure callbacks have been invoked during a layout pass. This is made available via the marker and event APIs.
Reviewed By: SidharthGuglani
Differential Revision: D15836983
fbshipit-source-id: 3835bef94e497375821c9f2ad8209447b4f11518
Summary:
There can be a race condition between bridge invalidation (hence TM binding invalidation) with pending Promise reject/resolve invocation. If invalidation happens first, invoking the resolve/reject from ObjC class might end up accessing destroyed PromiseWrapper, causing hard crash randomly.
The proper fix is to switch the objc promise resolve/reject block (objc block) to use C++ PromiseWrapper directly, such that the lifecycle of the shared_ptr<> can be correct.
Reviewed By: RSNara
Differential Revision: D15801403
fbshipit-source-id: 9b0c7d323b18d94e920ea3eafc3a6916dadba247
Summary: Make `TurboCxxModule::get` return `undefined` when trying to access methods that don't exist. This is what `TurboModule::get` does. So, it makes sense that this behaviour is preserved in TurboCxxModule.
Reviewed By: mdvacca
Differential Revision: D15780044
fbshipit-source-id: 13aeae081655735ef634f1dc09c0dc70a3a3a893
Summary:
This diff reimplements the prop parsing infrastructure in a part where it interacts with RawProps value.
Local synthetic tests show that the new way is 3x faster but the actual production result is quite unpredictable. MobileLab tests show some improvements about 10-20 ms on iPhone 6.
In short, the new way is faster because it inverts the lookup order and heavily relies on actual data types (and their properties) that we use. The old approach required about 130 hash-map lookups (where the key is `std::string`) to parse a single *Props object.
The new approach prepares concrete-props-specific tables with indexes of coming values ahead of time, iterates over raw data and puts it into those tables, and then performs a lookup in a very efficient manner.
Reviewed By: JoshuaGross
Differential Revision: D15752968
fbshipit-source-id: 847106e652eb7fc7ef7b99884a6f819ea3b9fd06
Summary: Even though it makes no sense to construct RootProps from RawProps, we need to support that (even if it's no-op) to be able to call this once from generic `ConcreteComponentDescriptor`. We will need it in the coming diff.
Reviewed By: mdvacca
Differential Revision: D15752970
fbshipit-source-id: b75a4023c5d0425a8dbe0f104a36a0f265eb6084
Summary:
In theory, those annotations can help the compiler to emit more optimized code and/or code that suggest CPU the proper way to utilize the instruction pipeline after the coming branch.
TBH, it's not clear how exactly beneficial those annotations are (esp. on target architectures) but they certainly don't hurt (in all cases here the favorite code branch is obvious).
Reviewed By: mdvacca
Differential Revision: D15752969
fbshipit-source-id: 8adf25a48107ffde828f735fb1386b30dbe63ede
Summary:
Previously we used `std::string` as a type behind all prop names (and name fragments). Even if `std::string` converted from `char const *` should be heavily optimized by STL and compiler, we still concatenate and copy those strings a lot. Switching to `char const *` allows avoiding tons of copying and inefficient equality checks.
Besides that, the future, more sophisticated optimization will rely on this.
Reviewed By: mdvacca
Differential Revision: D15511493
fbshipit-source-id: 9f509d18f0c737f7f77d4fea192d2ed1872e3731
Summary: This is a small perf tweak that alone does not give much improvement but the coming diffs will rely on the possibility to construct empty RawProps object.
Reviewed By: mdvacca
Differential Revision: D15511494
fbshipit-source-id: 39dec9336e6b5cf6ad33b1f3a06ca1c096ece628
Summary:
Address Sanitizer told me that I have UB in `RCTUIFontWeightFromFloat` and it was right. After a short investigation, I find out that the original assumption that `UIFontWeight` values are practically numbers from 100 to 900 was incorrect. In my simulator, it's something like from `-1` to `+1` (irregularly!). So, the whole subsystem worked only by accident.
This diff fixes that; now we never assume which actual values `UIFontWeight` constants have.
I will publish code style fixes as a separate diff (otherwise it will be really hard to review).
Reviewed By: JoshuaGross
Differential Revision: D15756620
fbshipit-source-id: d7f888e85815d863487c6b68a960e39fd473e095
sunnylqm
Samuel Susla
Ramanpreet Nara
David Vacca
Eric Williamson
Kevin Gozali
Sidharth Guglani
Joshua Gross
Championrunner
cpojer
James Treanor
Moti Zilberman
Emily Janzer
Will Holen
Min ho Kim
Peter Argany
Eli White
David Aurelio
Uladzislau Paulovich
Riley Dulin
Valentin Shergin
Ashok Menon
dattc2
zhongwuzw