Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36395
NativeState is a lighter-weight alternative to HostObject, which may simplify some of our use-cases of interacting with JS in the renderer. The down-side is that this approach doesn't allow exposing custom props or functions.
This diffs adds new feature flags to enable experimenting with this functionality and measure any perf impact.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D41553421
fbshipit-source-id: 3065bd7b60f0fa7b63c390e92a785582eee7e613
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36258
This fixes a few instances where YogaLayoutableShadowNode (or general shadownode casting) could offer better memory safety.
1. The reference form of traitCast() now terminates on invalid cast, instead of debug assert, since it is better to crash in production than to corrupt memory (which will crash somewhere later, in a much more confusing way).
2. We use traitCast() in more places where we previously would static_cast. This means needing to formally add a mutable version.
3. We bounds-check yoga children access in a single place by using `std::vector` `at()` instead of `[]`.
4. Removed `Trait::UnreservedTrait1` API, since multiple libraries using it can collide and we lose the memory safety benefits of `traitCast`.
This change is in response to a bug where `YogaLayoutableShadowNode` may perform an invalid `static_cast` of `RawTextShadowNode` if a text or number is rendered directly inside of a `<View>` (instead of a `<Text>` element).
This does not yet fix the underlying logic of YogaLayoutableShadowNode to act gracefully when a RawTextShadowNode makes its way into children. We just terminate, instead of corrupting memory.
Changelog:
[General][Breaking] - Better Fabric ShadowNode Memory Safety (Removes `Trait::UnreservedTrait` API)
Reviewed By: javache
Differential Revision: D43271779
fbshipit-source-id: 727c1230f72664bf4d261871c66ca61ddf0d5ffa
Summary:
changelog: [internal]
C++17 has implementation of shared_mutex in standard library. Let's use it instead of folly.
Reviewed By: cipolleschi
Differential Revision: D43275493
fbshipit-source-id: d766251226aa230110011aca94b4e697fe0d31a1
Summary:
changelog: [internal]
`useLayoutEffect` has two guarantees which React Native breaks:
- Layout metrics are ready.
- Updates triggered inside `useLayoutEffect` are applied before paint. State between first commit and update is not shown on the screen.
React Native breaks the first guarantee because it uses Background Executor. Background executor moves Yoga layout to another thread. If user core reads layout metrics in `useLayoutEffect` hook, it is a race. The information might be there, or it might not. They can even read partially update information. This diff does not affect this. We already have a way to turn off Background Executor. We haven't done this because it introduces regressions on one screen but we have a solution for that.
React Native breaks the second guarantee. After Fabric's commit phase, Fabric moves to mounting the changes right away. In this diff, we queue the mounting phase and give React a chance to change what is committed to the screen. To do that, we schedule a task with user blocking priority in `RuntimeScheduler`. React, if there is an update in `useLayoutEffect`, schedules a task in the scheduler with higher priority and stops the mounting phase.
We are not delaying mounting, this just gives React a chance to interrupt it.
Fabric commit phase may be triggered by different mechanisms. C++ state update, surface tear down, template update (not used atm), setNativeProps, to name a few. Fabric only needs to block paint if commit originates from React. Otherwise the scheduling is wrong and we will get into undefined behaviour land.
Rollout:
This change is gated behind `react_fabric:block_paint_for_use_layout_effect` and will be rolled out incrementally.
Reviewed By: javache
Differential Revision: D43083051
fbshipit-source-id: bb494cf56a11763e38dce7ba0093c4dafdd8bf43
Summary:
changelog: [internal]
Passing MountingCoordinator argument by value instead of reference. Using reference does not make sense since we eventually take ownership of shared_ptr anyway. This better communicates the intent.
Reviewed By: christophpurrer
Differential Revision: D43082955
fbshipit-source-id: 29e20abb9824c10a5f0d5e0ba1049ff6d67cee98
Summary:
Changelog: [Internal]
Caller needs to explicitly set commit options. This is for readability and making sure caller is aware of what are the options of the commit. This will be important in subsequent diff where we will add another commit option.
Reviewed By: christophpurrer
Differential Revision: D43082837
fbshipit-source-id: 1417205299c19430f902453c2b6d9bb9ca31707d
Summary:
This PR slightly improves the implementation of `dispatchCommand` method of `UIManagerBinding` to use existing variable `shadowNode` instead of calling `shadowNodeFromValue` again.
## Changelog
[INTERNAL] [CHANGED] - Eliminated double call of `shadowNodeFromValue` in `dispatchCommand`
Pull Request resolved: https://github.com/facebook/react-native/pull/35695
Test Plan: Launch RNTester with Fabric enabled and check if `scrollTo` or some other command works properly.
Reviewed By: christophpurrer
Differential Revision: D42233216
Pulled By: robhogan
fbshipit-source-id: db152206060ff599962f47c43fda8ea797f2a8cb
Summary:
changelog: [internal]
This is experimental implementation of `setNativeProps` in Fabric.
This diff brings `setNativeProps` to Fabric to make migration easier. I tried to stay as close as possible to Paper's behaviour, with all of its flaws (ready CAUTION section on https://reactnative.dev/docs/direct-manipulation)
State can't be stored in views because on iOS, eventually on Android, views are not the source of truth, shadow tree is. Fabric's implementation keeps state from setNativeProps in shadow node family in very inefficient data structure folly::dynamic. It is always reconciled with new prop updates. The performance cost is only paid in case node has used `setNativeProps` before.
Reviewed By: mdvacca
Differential Revision: D41875413
fbshipit-source-id: 453a5f7612a6f86a4cece269b13bd2ffd0c0e2d1
Summary:
This is a two step (1/2) fix to a race that could caused a `DELETE`...`CREATE` mutations being sent over to the fabric mounting layer. Such combination was assumed not possible from the differ, yet it happened at least in the existence of layout animation and when certain commits happen while animation is active.
This race condition could cause a view to get deleted at the end of one UI frame, yet the mount instructions generated from animation in the next frame still need to access the deleted view and caused crashes like T112157805. Note that even though such crash is recorded as `RetryableMountingLayerException` and is a soft crash (which only gets logged but not crash in production), the out-of-order mount instructions could lead to illegal view state and make the surface unusable, like what's shown here:
{F820669000}
The diff fixes this issue by removing the `DELETE` [conflict animation](https://fburl.com/code/5ctckvz3) keyframe, as well as the `CREATE` [immediate mutations](https://fburl.com/code/txyomytd) from the layout animation. The Fabric mounting layer assumes no combination of `DELETE...CREATE` in the same frame from differ + [layout animation overrides](https://fburl.com/code/zn17uqch).
Reviewed By: sammy-SC
Differential Revision: D41895427
fbshipit-source-id: d6df02663ba707af6db4a63a325ac776ca54d18e
Summary:
changelog: Introduce setNativeProps to Fabric
Add support for `setNativeProps` in Fabric for backwards compatibility. It is still recommended to move away from `setNativeProps` because the API will not work with future features.
We can make step [Migrating off setNativeProps](https://reactnative.dev/docs/new-architecture-library-intro#migrating-off-setnativeprops) in migration guide optional.
Reviewed By: yungsters, mdvacca
Differential Revision: D41521523
fbshipit-source-id: 4d9bbd6304b8c5ee24a36b33039ed33ae1fc21f8
Summary:
changelog: [internal]
Add findShadowNodeByTag_DEPRECATED` to Fabric's UIManager.
This API makes it possible to get ShadowNode reference for given native tag.
We will leverage this API to make UIManager.measure* calls backwards compatible in the new architecture.
Reviewed By: yungsters
Differential Revision: D41550734
fbshipit-source-id: a45b6a76634d034eac70603bad8ac60f9135ff24
Summary:
We want to fix the order of execution of layout, layout effects and passive effects in React Native, but the use of the Fabric background executor for layout complicates this (and other things).
This brings back a flag to disable this background thread to do layout synchronously in JavaScript, before the execution of layout effects and passive effects.
This is expected to regress performance on some screens, so we need to address the antipatterns in those screens before shipping this.
Changelog: [internal]
Reviewed By: javache
Differential Revision: D39727131
fbshipit-source-id: 4323b089234d3304ca3bfe5697668fb44ac64c12
Summary:
Changelog: [Internal] Remove unused RuntimeExecutor from UIManagerBinding constructor
Since [UIManager is the class that has a reference to RuntimeExecutor](https://github.com/facebook/react-native/blob/ce50c43986bae05ad62552be46f4d5bb4a46f097/ReactCommon/react/renderer/uimanager/UIManager.h#L35-L36), there's no reason for `UIManagerBinding` to own a reference to RuntimeExecutor too.
The purpose of `UIManagerBinding` is to bind the Fabric `std::shared_ptr<UIManager>` in C++ to `global.nativeFabricUIManager` in JS.
Doing this cleanup to make the next diff easier.
Reviewed By: sammy-SC
Differential Revision: D39490927
fbshipit-source-id: 4c393dccc6985bef44e3b0afc32b9f79bea25f80
Summary:
changelog: [internal]
Add more clang tidy rules to prevent common class of bugs.
Reviewed By: javache
Differential Revision: D39245194
fbshipit-source-id: 5521c5c4653d7005b96ebba494e810ba5075afbc
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/34403
This change mirrors D38457812 (https://github.com/facebook/react-native/commit/063c2b4668b279ccbca639f98f7a0a5c4d7c5690) which added -Wpedantic to ReactCommon targets, but for the Android build used by OSS. This should ensure contributors see the same warnings locally as the internal build would produce.
Changelog:
[Android][Changed] - Enable -Wpedantic in OSS Android Targets
Reviewed By: cortinico
Differential Revision: D38632454
fbshipit-source-id: 19a036ee3f902eb9d47c568aef448af9d8562358
Summary:
Folly's molly target combines a number of targets that are supposed to be useable on mobile. Since we're trying to move away from folly, we should instead list explicitly which parts of folly we're using so we can remove them over time, and track which targets no longer have any folly dependencies.
Changelog: [Internal]
Reviewed By: NickGerleman
Differential Revision: D38352060
fbshipit-source-id: 09d0d84793692f97f4d49390c99c38b23441df54
Summary:
React Native is compiled downstream with MSVC, meaning the introduction of code depending on language extensions specific to gcc/clang may cause breakage.
We can enable `-Wpedantic` to flag any behavior not officially supported by the specified C++ standard. This will includes rules beyond what MSVC has trouble with, but seems to not have too many "noisy warnings".
This change enables -Wpedantic in BUCK targets within ReactCommon.
This makes the OSS C++ build for Android/iOS slightly more permissive than the internal build, A followup is to add the changes to OSS build logic as well, to avoid contributors seeing more errors upon internal submission. (checking with cortinico on how to do this for Android).
react-native-windows uses a higher warning level than `-Wall`, which is an additional cause of compilation failures, but is not addressed as part of this change.
Changelog:
[Internal][Changed] - Enable -Wpedantic for targets inside ReactCommon
Reviewed By: rshest
Differential Revision: D38457812
fbshipit-source-id: 014da1ac0b7ad8f78154e6e447ed58def6bd0d47
Summary:
Fix todo and inconsistency across codebase. It's better to have just one way to refer to these types.
Changelog: [Internal]
Reviewed By: philIip
Differential Revision: D37653146
fbshipit-source-id: e82f09caa6cd6eec5512b78f413708d9c04a7a83
Summary:
If you don't list `Cxx` as a supported platform, we will use Android builds, even when using `buck run` for local execution.
Changelog: [Internal]
Reviewed By: derolf
Differential Revision: D37600464
fbshipit-source-id: 6ba8566cde4180524351c9d8c647ce1d4ac5279d
Summary:
LogBox was using AppRegistry to render on to the screen. Switch LogBox over to using SurfaceRegistry instead.
Changelog: [Internal]
Reviewed By: sshic
Differential Revision: D37223641
fbshipit-source-id: 59001ad290c1e2c2f14828d38a96f48bd1ab39ca
Summary:
This target is not a good idea for a number of reasons:
1. It groups up multiple targets which breaks the dependency graph
2. It does not handle dependency remapping correctly
3. It has no mirror into fbcode
We should warn people this is a bad idea
Reviewed By: alexmalyshev
Differential Revision: D36519357
fbshipit-source-id: d60ca3237c7710118732578fecd1b2fc8903321b
Summary:
This diff cleans up several Android Makefiles which we're not using anymore
as they've been replaced by CMake files.
There are still 3 Makefiles left, which I'm aiming to remove in the near future.
Changelog:
[Internal] [Changed] - Remove unused Makefiles from React Native core
Reviewed By: javache
Differential Revision: D36660902
fbshipit-source-id: 8afffac74d493616b0f9414567821cd69f4ef803
Summary:
Cherry picking https://github.com/facebook/react-native/pull/33707 to main branch
This change is extending the changes made by alespergl to reduce the file paths and command lengths of ndk build commands
Essentially we are shortening the length of the source files by using relative paths instead of absolute paths as enumerated by the wildcard expression
This commit is extending the fix by including all the new modules introduced into RN for the new architecture, including the generated modules.
We are also reverting the ndk bump as ndk23 is crashing frequently when building RN with new arch. The reduced file paths lengths ensures the ndk bump is not required for relatively short application paths.
Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources
## Changelog
Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources
[CATEGORY] [TYPE] - Message
Pull Request resolved: https://github.com/facebook/react-native/pull/33784
Test Plan: Verified building on windows box
Reviewed By: javache
Differential Revision: D36241928
Pulled By: cortinico
fbshipit-source-id: 1ce428a271724cbd3b00a24fe03e7d69253f169b
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/33688
These methods used to be public in the legacy implementation, and hiding them significantly reduces amount of customization available to other clients outside Fabric core.
Changelog: [Internal] Allow external callers to call UIManager methods
Reviewed By: cipolleschi
Differential Revision: D35818114
fbshipit-source-id: 4dc4177c82b5db9ae3d136a1a83f5ec3123b971f
Summary:
Now that the PFH node has been renamed this updates the pfh label.
Produced via `xbgs -l -e '"pfh:ReactNative_CommonInfrastructurePlaceholde"' | xargs sed -i 's/"pfh:ReactNative_CommonInfrastructurePlaceholde"/"pfh:ReactNative_CommonInfrastructurePlaceholder"/'`
Reviewed By: jkeljo
Differential Revision: D35374087
fbshipit-source-id: 61590f69de5a69ec3b8a0478f6dd43409de3c70b
Summary:
While it would be better to be able to do all of the ownership metadata at the Buck macro level, that proved to be more work than expected.
This diff adds the corresponding pfh label to all targets in `xplat/js/react-native-github` that have a Supermodule label. Once the migration is complete the Supermodules labels will be able to be removed.
Reviewed By: cortinico
Differential Revision: D35221544
fbshipit-source-id: d87d5e266dfb5e6ee087251dc34dff5db299bbaf
Summary:
Adding more context for event dispatching process in systrace.
Changelog: [Internal]
Reviewed By: philIip
Differential Revision: D35208257
fbshipit-source-id: 4a70e15a0074d4a53a895066e6fa1e60a6ebda0d
Summary:
This PR fixes RTTI (run-time type information) for ShadowNodeWrapper and ShadowNodeListWrapper classes, i.e., calls to dynamic_cast and dynamic_pointer_cast that are called via JSI's getHostObject calls.
The fix is simply to add a so-called "key function" in a form of virtual destructor. Key functions needs to be a virtual non-pure and non-inlined functions that points the compiler as to which library contains the vtable/type information for a given class (see https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable and https://developer.android.com/ndk/guides/common-problems#rttiexceptions_not_working_across_library_boundaries)
Without the "key function", calls to dynamic_cast for ShadowNodeWrapper instances won't work across library boundaries because the class will have separate definitions in each separate library, therefore objects created in one of those libraries won't be recognized as the same type by the other library. This has been a problem in reanimated and gesture-handler libraries where we call `object.getHostObject<ShadowNodeWrapper>(rt)` (this is a method from JSI) in order to access ShadowNode instance from a handle we have in JS. I think, this issue is going to be relevant to more libraries that cope with view instances. In this scenario, we have a separate library, say "libreanimated.so" that calls to `getHostObject` which is an inline function that calls `dynamic_cast` for the `ShadowNodeWrapper` class. On the other hand, the instances of `ShadowNodeWrapper` are created by the code from `libreact_render_uimanager.so`. Because of that `dynamic_cast` fails even though it is called on instance of `ShadowNodeWrapper` because the class has separate vtable/type info: one in `libreanimated.so` and one in `libreact_render_uimanager.so` (by "fails" I mean that it actually returns `nullptr`).
This problem has been documented here: https://developer.android.com/ndk/guides/common-problems#rttiexceptions_not_working_across_library_boundaries where the solution is for the class to have a so-called "key function". The key function makes it so that compiler sees that one of the implementation for a given class is missing and therefore can safely assume that a vtable/type info for a given class is embedded into some library we link to.
This change adds a virtual destructor that is declared in the header file but defined in file that gets compiled as a part of `libreact_render_uimanager`. As a result, the compiler only creates one vtable/type info and calls to dynamic_cast works as expected in all libraries for `ShadowNodeWrapper` and `ShadowNodeListWrapper` classes.
This issue would only surface on Android, because on iOS all libraries by default are bundled together via Pods, whereas on Android each library is loaded separately using dynamic loading.
## Changelog
[Fabric][Android specific] - Fix dynamic_cast (RTTI) for ShadowNodeWrapper and similar classes when accessed by third-party libraries.
Pull Request resolved: https://github.com/facebook/react-native/pull/33500
Test Plan:
1. In order to test this you need to add a library that'd include `<react/renderer/uimanager/primitives.h>` (i.e. use this branch of reanimated library: https://github.com/software-mansion/react-native-reanimated/tree/fabric)
2. After compiling the app inspect libreact_render_uimanager.so and libreanimated.so artifacts with `nm` tool
3. Notice that symbols like `vtable for facebook::react::ShadowNodeWrapper` and `typeinfo for facebook::react::ShadowNodeWrapper` are only present in the former and not in the latter library (before this change you'd see them both)
Reviewed By: ShikaSD
Differential Revision: D35143600
Pulled By: javache
fbshipit-source-id: 5fb25a02365b99a515edc81e5485a77017c56eb8
Summary:
This Diff moves from specifying a list of files to use file(GLOB) with
CONFIGURE_DEPENDS on several CMakefiles.
I've updates those where we use globbing also inside buck.
Changelog:
[Internal] [Changed] - Setup Globbing with CONFIGURE_DEPENDS inside CMake files
Reviewed By: ShikaSD
Differential Revision: D34826311
fbshipit-source-id: 8fc654626c897cdc4cdd79c699ce19f1e5e1212f
Summary:
Rearranges folly_futures configuration into a static library only required for `hermes-inspector` + `folly_runtime` which merges `folly_json` and mutex-related implementations `folly_futures` was used for. As `hermes-executor-debug` is removed by `vmCleanup` configurations later, it allows to shave additional 300KB from the release APK size.
Changelog: [Internal] - Rearrange folly build to reduce APK size
Reviewed By: cortinico
Differential Revision: D34342514
fbshipit-source-id: b646680343e6b9a7674019506b87b96f6007caf2
Summary:
This is the first round of CMake files to support the React Native build on Android.
They're supposed to eventually replace the various Android.mk files we have around in the codebase.
So far we're not actively using them. This is the first step towards migrating our
setup to use CMake
Changelog:
[Internal] [Changed] - First Round of CMake files for React Android
Reviewed By: ShikaSD
Differential Revision: D34762524
fbshipit-source-id: 6671e203a2c83b8874cefe796aa55aa987902a3b
Summary:
changelog: [internal]
pass raw ShadowNode instead of shared_ptr. Ownership is not transferred, shared_ptr is misleading.
Reviewed By: javache
Differential Revision: D33917010
fbshipit-source-id: 4d9fdd4b4e0376149f1719ad160b957de4afdce3
Summary:
changelog: [internal]
You can read more about this rule on https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
# Isn't it wasteful to copy? Isn't reference more efficient?
This rule of thumb is no longer true since C++11 with move semantics. Let's look at some examples.
# Option one
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
private:
std::string text_;
};
```
By using reference here, we prevent the caller from using rvalue to and avoiding copy. Regardless of what the caller passes in, copy always happens.
# Option two
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
TextBox(std::string &&text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, we provide two constructors, one for const reference and one for rvalue reference. This gives the caller option to avoid copy. But now we have two constructors, which is not ideal.
# Option three (what we do in this diff)
```
class TextHolder
{
public:
TextBox(std::string text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, the caller has option to avoid copy and we only have single constructor.
Reviewed By: fkgozali, JoshuaGross
Differential Revision: D33276841
fbshipit-source-id: 619d5123d2e28937b22874650366629f24f20a63
Summary:
Renaming the `better` utilities to `butter`:
- to prevent claims that this library is superior to others - it really depends on use cases
- to indicate ease of use throughout the codebase, easily spread like butter
Changelog: [C++][Changed] Renaming C++ better util to butter, used by Fabric internals
Reviewed By: JoshuaGross
Differential Revision: D33242764
fbshipit-source-id: 26dc95d9597c61ce8e66708e44ed545e0fc5cff5
Summary:
changelog: [internal]
Just moving code that doesn't belong to UIManagerBinding out of the class.
Reviewed By: philIip
Differential Revision: D33060412
fbshipit-source-id: 2d54929072cef14fd1fa6b70bde382ae21ecff45
Summary:
Applies suggestions from default preset that make sense in our codebase
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D33010231
fbshipit-source-id: 6bc9edf01fd9bd9938d211e3494dd1a127b24eaa
Summary:
changelog: [internal]
Provide `UIManager` to `UIManagerBinding` in constructor to make the API safer.
Reviewed By: philIip
Differential Revision: D32668892
fbshipit-source-id: a15cd295196a60c3f46997e59c05c4f90503e18d
Summary:
The IDE warning suggests that passing folly::dynamic by value will create a copy on each call.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D32978154
fbshipit-source-id: a47a60c332a9d299eb2110d3537dfab0bc2398b6
Summary:
changelog: [internal]
Exposes a new flag on RuntimeScheduler: `unstable_getIsSynchronous`. Flag indicates if the current code is run synchronously and therefore commit phase should be synchronous.
Unit tests will be added later, to keep this diff short. This code path is not executed yet.
Reviewed By: mdvacca, ShikaSD
Differential Revision: D32677814
fbshipit-source-id: e01d4fff7e716d627ff99fe104965851138c3aef
Pieter De Baets
Nick Gerleman
Samuel Susla
Rubén Norte
Tomek Zawadzki
Xin Chen
Phillip Pan
Jonathan Keljo
Rubén Norte
Paige Sun
Samuel Susla
Nick Gerleman
Pieter De Baets
Ramanpreet Nara
Michael Lee (Engineering)
Nicola Corti
Anandraj Govindan
Chris Olszewski
Xin Chen
CodemodService Bot
Krzysztof Magiera
Andrei Shikov
Andres Suarez
Kevin Gozali