Summary:
I had intended to make this change as part of the stack I landed earlier, but I had some poorly resolved merge conflicts that left this path disabled.
I verified that T76057501 no longer repros and ran unit tests.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27788467
fbshipit-source-id: 42148b887c6b3c0e815f1805e6bfb3ee58503e48
Summary:
Add mounting layer test that stress-tests differ on (un)flattening.
This fails before D27759380 and D27730952, and passes after.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27767219
fbshipit-source-id: a7e186e510f95792da6f98f80fcae5ff8ac74775
Summary:
While I think this was a very marginal bug with no known issues in the wild, incorrect layout values were sometimes being propagated to certain nodes. This would only occur during complex nested (un)flattening operations and may only impact node consistency, specifically with setting the "previous" ShadowView of mutation instructions, specifically REMOVE and DELETE. Even in rigorous testing I had trouble hitting this case and it didn't seem to impact the "next" values in CREATE, INSERT, or UPDATE.
The issue: previously `sliceChildShadowNodeViewPairsV2` assumed that the node it's operating on is a child of a non-flattened view, and the baseline origin is `{0,0}`. You can see when `sliceChildShadowNodeViewPairsRecursivelyV2` is called, a `layoutOffset` is passed in. If we ever got a list of a node that was in a flattened parent by calling `sliceChildShadowNodeViewPairsV2`, we would incorrectly assume that baseline layoutOffset for the node is `0,0`.
Now, we store the layoutOffset in the ShadowViewNodePair and can retrieve it when getting child pairs of a node.
Changelog: [internal]
Reviewed By: sammy-SC
Differential Revision: D27759380
fbshipit-source-id: a89756190a1cb377bcc55ff31799c2afbaecdaa9
Summary:
Previously, `ShadowViewNodePair::List` owned each `ShadowViewNodePair` but whenever we put `ShadowViewNodePair` into a TinyMap, those were unowned pointer references. This worked... 99% of the time. But in some marginal cases, it would cause dangling pointers, leading to difficult-to-track-down issues. So, I'm moving both of these to be unowned pointers and keeping a `std::deque` that owns all `ShadowViewNodePair`s and is itself owned by the main differ function. See comments for more implementation details. I'm moderately concerned about memory usage regressions, but practically speaking this will contain many items when a tree is created for the first time, and then very few items after that (space complexity should be similar to `O(n)` where `n` is the number of changed nodes after the last diff).
See comments as to why I believe `std::deque` is the right choice. Long-term there might be data-structures that are even more optimal, but std::deque has the right tradeoffs compared to other built-in STL structures like std::list and std::vector, and is probably better than std::forward_list too. Long-term we may want a custom data-structure that fits our needs exactly, but std::deque comes close and is possibly optimal.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27730952
fbshipit-source-id: 2194b535439bd309803a221188da5db75242005a
Summary:
I am fixing an extremely marginal case that probably impacts nothing in production, but in theory, could - see next diff in stack for the assert that is being hit.
TL;DR in marginal, complex cases with a lot of un/flattening, we can generate the following sequence of mutations:
```
UPDATE node V1 -> V2
REMOVE node V1
```
That is incorrect, and what we actually want is:
```
UPDATE node V1 -> V2
REMOVE node V2
```
While this, again, impacts /nothing/ in prod that we know of, it would be good to get this correct so that we can enable stricter asserts (see next diff).
This will also help with debugging LayoutAnimations.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27697788
fbshipit-source-id: 47f34fe3e8107167b3df4db841d2cc14c58cb31d
Summary:
Changes in following diffs will be gated by this feature flag.
The differ in the new file is copied from the current stable implementation and will not be modified until it's deleted.
Changelog: [Internal]
Reviewed By: sammy-SC, mdvacca
Differential Revision: D27775698
fbshipit-source-id: 03d9518ffd2b1f25712386c56a38bd2b4d839fc2
Summary:
First, I make the breadcrumbs mechanism (landed just this week) more readable - I forgot to add separators between the breadcrumbs.
Second, there is a path that I am 99% sure we never hit. I've had comments to that effect for a ~year, but now I'm adding a falsey assert. If we don't hit it in prod after a few months I'll be more comfortable just deleting the branch entirely (while probably keeping the assert).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27697786
fbshipit-source-id: 6d74d1703b2212d069fbed510f2655ec17294458
Summary:
Move this assert so the debug logging executes first; in the case where this assert fires, we'll get a little more information (for Android, where attaching a Cxx debugger is a bit harder vs iOS).
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27585132
fbshipit-source-id: e3f4cc3d78587744b9e73db685eda1fd6c36ca9d
Summary:
With all known remaining issues (outside of LayoutAnimations, potentially) resolved, enable this assert.
Note for future readers: this is the first time this particular assert has EVER been enabled widely, so if we hit this assert in dev, it's great signal for debugging BUT does not necessarily indicate any wide-spread problems in prod, or with any subsystems. This will simply help us become "more correct" over time.
It is possible, but not extremely likely, that cleaning up things that cause this assert will improve stability/crashes.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27697787
fbshipit-source-id: 28fa34eba70548f5001bbd47f41dbe3c6ff3b4c1
Summary:
In order to move away from the legacy system (bridge etc), we need to decouple the new architecture assumptions from it. This flag and assertion functions will help track the runtime and report violations along the way. The goal is to have 0 violation before switching over to the pure new architecture.
Note: this is not used right now.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D27783246
fbshipit-source-id: 61f0d77c129bddcde7f24a803432f2d359c5bff3
Summary:
Log a little more information in the iOS-only assert checking that the REMOVE index is correct.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27632457
fbshipit-source-id: 56e668dc2f22841e3cf3d838ba605d63cc7c1bad
Summary:
Allow conversion of LayoutMetrics to DebuggableString.
We also skipped a field in comparison. It probably isn't impactful in terms of production issues, but still wasn't correct.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27709451
fbshipit-source-id: 987fc2de0a4562a295d6cbeffdd922cbf056b811
Summary:
When the React Native Cmd+D menu is opened, something re-runs module initialization code (including DevTools backend initialization) which recreates the `WebSocket` and kills any already-connected frontend.
It's not clear to me why this is done. (Intentional? Accident?) But it makes it difficult to connect the React Native Inspector and DevTools together without multiple reloads. This Diff prevents the Cmd+D menu from killing the `WebSocket` connection as a workaround. A better long-term fix would (probably) be to not eagerly re-run these modules.
## Changelog
[General] [Fixed] - Don't disconnect DevTools WebSocket connection on Cmd+D
Reviewed By: fkgozali, sammy-SC
Differential Revision: D27742376
fbshipit-source-id: 60ab3e4763da6b055c28c7aafc6d460e7f4a601d
Summary:
Changelog: [internal]
To prevent starvation, this diff implements expiration time as a way to order tasks in priority queue. This stops higher priority tasks from preventing lower priority tasks from running. The same mechanism is implemented in [JavaScript's scheduler](https://github.com/facebook/react/blob/master/packages/scheduler/src/forks/SchedulerNoDOM.js).
Reviewed By: mdvacca
Differential Revision: D27707887
fbshipit-source-id: 3dc734c856a166ef5c17c5045ebd429565ba79f0
Summary:
Changelog: [internal]
Implement RuntimeScheduler::getShouldYield and expose it through JSI.
For now we are only returning `false`. The value is backed by atomic_bool and in the future we will be able to indicate that React should yield to native.
JavaScript implementation:
https://github.com/facebook/react/blob/master/packages/scheduler/src/forks/SchedulerNoDOM.js#L439-L441
Reviewed By: JoshuaGross
Differential Revision: D27648579
fbshipit-source-id: b9313e2efbd9daae8975357df9de803f24a35e89
Summary:
To fix [the CI error](https://app.circleci.com/pipelines/github/facebook/react-native/8708/workflows/d584348e-941f-4653-96c2-46375894dfaa/jobs/196410)
There are two errors:
`error: undefined reference to '__android_log_write'` which solved by adding `-llog` linker flag.
`ld: error: cannot find -lc++` which workaround by static linking.
For the root cause, I am thinking that is after [NDK r19](https://developer.android.com/ndk/guides/other_build_systems),
the `-target` should specificy api level.
However, buck does not add this accordingly, e.g. `-target armv7-none-linux-androideabi`
Given wrong target will make NDK to have `cannot find -lc++` error.
The workaround is to use static linking.
Since it was an oss_cxx_library, the change should not have impact to Facebook internal testing.
## Changelog
[Internal] [Fixed] - Fix CI "Build Tests: Android Instrumentation Tests" errors
Pull Request resolved: https://github.com/facebook/react-native/pull/31352
Test Plan: Make CI green
Reviewed By: JoshuaGross
Differential Revision: D27757838
Pulled By: fkgozali
fbshipit-source-id: 8f9c80a89c6240938218abacb8a82e3e2e71adbc
Summary:
Changelog: [internal]
unstable_scheduleCallback needs to return reference to the created task so it can be cancelled later.
Reviewed By: mdvacca
Differential Revision: D27622779
fbshipit-source-id: 54160015c7f98e123d08c2e13efac4f498d3ba5e
Summary:
Changelog: [internal]
Add minimal implementation of schedule task. More features and proper scheduling will be added later.
Reviewed By: mdvacca
Differential Revision: D27622138
fbshipit-source-id: b2e4623d38e7217290a6a3c59ccc10a1c13e3a4f
Summary:
Changelog: [internal]
`glog` needs to be exported dependency because it is used in public headers.
Reviewed By: mdvacca
Differential Revision: D27617632
fbshipit-source-id: 91aa27b641286002a80a8cd5ef2e6fe6c266b452
Summary:
Changelog: [Internal]
Calls to `surfaceHandler.start()` and `setDisplayMode(DisplayMode::Visible)` in quick succession on different threads can cause a race condition between mount and commit operations.
The mountingCoordinator will try to mount an empty revision without any commits causing it to fail with:
```
TransactionTelemetry.cpp:108: function getCommitStartTime: assertion failed (commitStartTime_ != kTelemetryUndefinedTimePoint)
```
which is called from [Binding.cpp](https://www.internalfb.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp?lines=791-791&blame=1).
This change avoids this initial commit by verifying we had at least 1 revision commited before mounting it.
Reviewed By: sammy-SC
Differential Revision: D27430174
fbshipit-source-id: d208d55f02cd218a316d6fea62d0106c2bcb4be8
Summary:
Changelog: [Internal]
Inverts registration of a SurfaceHandler with the scheduler: instead of passing a scheduler to the SurfaceHandlerBinding, we can now query the SurfaceHandler and register it in place.
Reviewed By: JoshuaGross
Differential Revision: D27624541
fbshipit-source-id: db5d7f1375fad72a805309a3fcd5a33080e4a4a7
Summary:
Changelog: [Internal]
Links APIs in Fabric and Venice to create a surface without a view and mount it separately when surface is started the usual way.
Reviewed By: mdvacca
Differential Revision: D27339365
fbshipit-source-id: d1b674ce856957465eb6f3a5d7f26eb0ab625353
Summary:
Changelog:
[Internal][Added] - Use injected Modal if set
Due to Modal uses AppContainer in a dev env, in the StrictMode we'll get warnings about legacy context API usage from inside the AppContainer.
It's a gnarly change to get rid of that legacy context so I'll be following up with additional code change prep steps towards the eventual removal of the legacy context.
Reviewed By: lunaleaps
Differential Revision: D27611218
fbshipit-source-id: f79475845e763527fff9ecfdbf3e581c4666e015
Summary:
Deprecates `EventEmitter#removeSubscription`. This required temporarily introducing a new `__removeSubscription` method that is only invoked by `EmitterSubscription`. This is necessary so that we do not completely break usages of `EventEmitter` that are supplying constructor arguments (which is also deprecated, but still supported until the next release).
Calling this method will now cause a warning to appear with instructions to instead invoke `remove()` on the subscription itself.
Lastly, changed `console.error` deprecation notice to instead use `console.warn`. This is in line with the principle that errors are "broken today" and warnings will be "broken tomorrow".
Changelog:
[General][Deprecated] - `EventEmitter#removeSubscription` is now deprecated.
Reviewed By: rubennorte
Differential Revision: D27704279
fbshipit-source-id: 581f5b2ab46b1bcfc1d20898b3d3392988dccbd5
Summary:
Cleans up some minor redundant internal types in `_EventEmitter.js`.
Changelog:
[Internal]
Reviewed By: rubennorte
Differential Revision: D27712301
fbshipit-source-id: e6affcb7948e0f47a1775b74b12f59ec2a6abddb
Summary:
Deprecates the constructor argument to `EventEmitter`.
It was only ever used by React Native to instantiate a `NativeEventEmitter` with the same "vendor" as `RCTDeviceEventEmitter`, which was a gross violation of encapsulation that led to an overcomplicated implementation of `EventEmitter`. I am now untangling that mess.
Changelog:
[General][Deprecated] - It is now deprecated to pass a constructor argument to `EventEmitter(...)`.
Reviewed By: rubennorte
Differential Revision: D27704182
fbshipit-source-id: 102f26a8a9c029f0e7a1346dc2f2d9f2548d6a42
Summary:
Changelog:
[Internal] - Refactors SectionList component to be a functional component instead of a Class component
Closes#31322
Reviewed By: lunaleaps
Differential Revision: D27681457
fbshipit-source-id: ef0e68dd4bc91e83b40c55a4a959f27e69292964
Summary:
Changelog: [internal]
For React Native classic the behaviour is identical.
# Problem
`ScrollView.scrollResponderScrollNativeHandleToKeyboard` is broken in new React Native renderer. The behaviour depends on race between two events, `measureLayout` and `keyboardWillShow` event. In the new renderer, `measureLayout` always wins because it is a synchronous call, unlike in React Native classic.
To fix this we can just delay execution by wrapping it in `setTimeout`, which puts it on the back of the queue.
Reviewed By: JoshuaGross
Differential Revision: D27705883
fbshipit-source-id: 7556f86978ca3ccd2fddf1eef2435af26b54f585
Summary:
Changelog: [Internal]
`NativeAnimatedModule` on Android currently enforces all animation operations to be processed in batches to ensure that all associated operations are processed at the same time.
Some operations, however, can be triggered outside of the batching calls (e.g. when using `Animated` for tracking touches `PanResponder`), and they are not processed until the next batch.
This change tracks if we are currently processing a batch and doesn't assign a batch number if an operation was triggered outside of `startOperationBatch`/`finishOperationBatch` pair.
Reviewed By: mdvacca
Differential Revision: D27682424
fbshipit-source-id: 2ea8737c353c81557fa586b15aa5760db3e8813f
Summary:
## Context
In native modules implementing an event emitter, we can wait for JS to subscribe to an event before making the subscription to the right native API in the native module. This is only supported on iOS at the moment and we want to support it on Android too, so we can manage resources more efficiently and avoid custom code to do this on Android, like this:
https://www.internalfb.com/intern/diffusion/FBS/browse/master/xplat/js/RKJSModules/public/Dating/Profile/ProfileView/ProfileGemstoneProfileView.js?commit=165ad219e6bf&lines=302-304
The way this works now is by creating instances of `NativeEventEmitter`, where we pass a reference to the native module that needs to be notified when there are new subscriptions. We have explicit code to ignore this native modules in all platforms except for iOS:
https://www.internalfb.com/intern/diffusion/FBS/browsefile/master/xplat/js/react-native-github/Libraries/EventEmitter/NativeEventEmitter.js?commit=5a1e671453465e844dd851c458cb2467a2db5d03&lines=44-52
## Changes
This removes the check for iOS from `NativeEventEmitter` so we also try to use the native module to notify subscriptions on Android.
We have migrated all existing code passing a native module to `NativeEventEmtiter` to only pass it on iOS, so we don't change this behavior in existing code.
Any other existing code using this API is most likely fine too. It didn't work before so the expectation is that the native module wouldn't be implemented on Android anyway.
Changelog: [Android][Changed] - Modified `NativeEventEmitter` to also use the passed native module to report subscriptions on Android
Reviewed By: yungsters
Differential Revision: D27500994
fbshipit-source-id: ef82da04020fb08cd0ea4f1cfffd1da6453ab0b9
Summary:
This diff adds a new variable called "DisplayMode" into SurfaceHandler.cpp and FacebookAppRouteHandler.js. The purpose of DisplayMode is for the native pre-render system to notify React that the a surface is either being "pre-rendered" or "rendered"
When the surface is being "pre-rendered" (displayMode == "SUSPENDED"), react will create and commit React Trees, but it will not execute use-effect callbacks
When the surface is being "rendered" (displayMode == "VISIBLE"), react will create and commit React Trees and it will not execute all use-effect callbacks that weren't executed during "pre-rendering"
By default surfaces are going to be rendered with displayMode == "VISIBLE".
This diff should not create any change of behavior for now, this is the infra required to integrate the new offScreen API the react team is working on for pre-rendering system
changelog: [internal] internal
Reviewed By: yungsters
Differential Revision: D27614664
fbshipit-source-id: f1f42fdf174c2ffa74174feb1873f1d5d46e7a95
Joshua Gross
CodemodService FBSourceClangFormatLinterBot
Samuel Susla
Kevin Gozali
Brian Vaughn
Kacie Bawiec
Jorge Rossi
Peter Argany
David Vacca
Kudo Chien
Mike
Andrei Shikov
Nadiia D
Tim Yung
Madhav Gharmalkar
Rubén Norte