Summary:
This diff adds support elevation prop in Fabric core, additionally it adds this prop as non collapsable on the view flattening algorithm
changelog: [Internal] internal change in fabric to support elevation prop
Reviewed By: JoshuaGross
Differential Revision: D21896465
fbshipit-source-id: e0854acc0b2ac30eaf3f82d615aab1cf378cc530
Summary:
Changelog: Move collapsable to Shared View props
I'm researching why `collapsable` isn't passed to Fabric UIManager in production build, I discovered that collapsable was Android only prop but in Fabric we use it in iOS as well.
This doesn't fix the collapsable not being passed through but I think this makes code semantically correct.
Reviewed By: mdvacca
Differential Revision: D21862770
fbshipit-source-id: 492c6a15955b907dbbeb8530c81f6a868cf379c3
Summary: TurboModules doesn't cache invocations of getConstants(). This diff looks at which NativeModules' getConstants() methods gets repeated called in Marketplace, and caches those invocations in JS.
Reviewed By: fkgozali
Differential Revision: D21874123
fbshipit-source-id: a44b98b3ac8621f67c9c0f3b7c4003a561d1e15d
Summary: TurboModules doesn't cache invocations of getConstants(). This diff looks at which NativeModules' getConstants() methods gets repeated called in Marketplace, and caches those invocations in JS.
Reviewed By: fkgozali
Differential Revision: D21874124
fbshipit-source-id: 03d2318e1b6d00236ef707f9f19a640bf8c08786
Summary:
Noticed this hanging around when reading MessageQueue, delete it.
Changelog: [internal]
Reviewed By: fkgozali
Differential Revision: D21874609
fbshipit-source-id: 627c6a18cdb859a1aaf2eb596f8744156d18393c
Summary:
JSError creation can lead to further errors. Make sure these cases
are handled and don't cause weird crashes or other issues.
This solution has a few parts:
* include a ScopedNativeDepthTracker in checkStatus
* If an exception object message or stack property is already a String, don't
call JS String ctor on it
* Verify that a jsi::Value is a String before calling getString on it.
* Add more tests for JSError construction
Changelog: [Internal]
Reviewed By: dulinriley
Differential Revision: D21851645
fbshipit-source-id: 2d10da0e741ad4ede93cd806320f68ad512e5138
Summary:
There are two ways to make a NativeModule method execute synchronously:
- Declare the NativeModule method to be synchronous (i.e: use `RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD`).
- Make the NativeModule synchronous (i.e: make its method queue `RCTJSThread`). This way, all its methods are synchronous.
`RCTNativeModule` executes all synchronous methods on the JS thread:
- Executing an async methods on a sync module: https://git.io/JfRPj
- Executing a sync method: https://git.io/JfRXe
However, in TurboModules we block the JS thread, execute the method on the NativeModule's method queue, and then unblock the JS thread. While this approach is thread-safe, and arguably the correct way to dispatch sync methods, it's also much slower than the alternative. Therefore, this diff migrates the legacy behaviour to the TurboModule system.
## Special Case: getConstants()
When an ObjC NativeModule requires main queue setup, and it exports constants, we execute its `constantsToExport` method on the main queue (see: [RCTModuleData gatherConstants](https://github.com/facebook/react-native/blob/c8d678abcf93fd3f6daf4bebfdf25937995c1fdf/React/Base/RCTModuleData.mm#L392-L402)). I replicated this behaviour with TurboModules.
Changelog:
[iOS][Fixed] - Execute ObjC TurboModule async method calls on JS thread for sync modules
Reviewed By: fkgozali
Differential Revision: D21602096
fbshipit-source-id: 42d07b7ad000abeac27091dc3ec440e3836d2eae
Summary:
## Context
- If a NativeModule requires main queue setup, its `constantsToExport` method is executed on the main queue.
- In the TurboModule system, `constantsToExport` or `getConstants` is treated like a regular synchronous NativeModule method. Therefore, it's always executed on the JS thread.
This difference in behaviour is dangerous when we're A/B testing the TurboModule infra: One could write a NativeModule that requires main queue setup, and have it expose constants that access objects/state only accessible on the UI thread. This NativeModule would work fine in the legacy infra, which could be the case if the NativeModule author is testing locally. But once it ships to prod, it may run with the TurboModule system, and crash the application. To mitigate this risk, I'm removing this special main queue execution of `constantsToExport` from the legacy infrastructure.
## Consequences
- If a NativeModule's `constantsToExport` method accesses objects/state only accessible on the UI thread, it must do so by explicitly scheduling work on the main thread. I wrote up a codemod to fix this for our OSS modules: D21797048.
- Eagerly initialized NativeModules that required main queue setup had their constants calculated eagerly. After the changes in this diff, those NativeModules will have their constants calculated lazily. I don't think this is a big deal because only a handful of NativeModules are eagerly initialized, and eagerly initialized NativeModules are going away anyway.
Changelog:
[iOS][Removed] - Main queue execution of constantsToExport in NativeModules requiring main queue setup
Reviewed By: fkgozali
Differential Revision: D21829091
fbshipit-source-id: df21fd5fd2ef45a291c07400f360bba801ae290f
Summary:
If a NativeModule requires main queue setup, its `getConstants()` method must be executed on the main thead. The legacy NativeModule infra takes care of this for us. With TurboModules, however, all synchronous methods, including `getConstants()`, execute on the JS thread. Therefore, if a TurboModule requires main queue setup, and exports constants, we must execute its `getConstants()` method body on the main queue explicitly.
**Notes:**
- The changes in this diff should be a noop when TurboModules is off, because `RCTUnsafeExecuteOnMainQueueSync` synchronously execute its block on the current thread if the current thread is the main thread.
- If a NativeModule doens't have the `requiresMainQueueSetup` method, but has the `constantsToExport` method, both NativeModules and TurboModules assume that it requires main queue setup.
## Script
```
const exec = require("../lib/exec");
const abspath = require("../lib/abspath");
const relpath = require("../lib/relpath");
const readFile = (filename) => require("fs").readFileSync(filename, "utf8");
const writeFile = (filename, content) =>
require("fs").writeFileSync(filename, content);
function main() {
const tmFiles = exec("cd ~/fbsource && xbgs -n 10000 -l constantsToExport")
.split("\n")
.filter(Boolean);
const filesWithoutConstantsToExport = [];
const filesWithConstantsToExportButNotGetConstants = [];
const filesExplicitlyNotRequiringMainQueueSetup = [];
tmFiles
.filter((filename) => {
if (filename.includes("microsoft-fork-of-react-native")) {
return false;
}
return /\.mm?$/.test(filename);
})
.map(abspath)
.forEach((filename) => {
const code = readFile(filename);
const relFilename = relpath(filename);
if (!/constantsToExport\s*{/.test(code)) {
filesWithoutConstantsToExport.push(relFilename);
return;
}
if (!/getConstants\s*{/.test(code)) {
filesWithConstantsToExportButNotGetConstants.push(relFilename);
return;
}
if (/requiresMainQueueSetup\s*{/.test(code)) {
const requiresMainQueueSetupRegex = /requiresMainQueueSetup\s*{\s*return\s+(?<requiresMainQueueSetup>YES|NO)/;
const requiresMainQueueSetupRegexMatch = requiresMainQueueSetupRegex.exec(
code
);
if (!requiresMainQueueSetupRegexMatch) {
throw new Error(
"Detected requiresMainQueueSetup method in file " +
relFilename +
" but was unable to parse the method return value"
);
}
const {
requiresMainQueueSetup,
} = requiresMainQueueSetupRegexMatch.groups;
if (requiresMainQueueSetup == "NO") {
filesExplicitlyNotRequiringMainQueueSetup.push(relFilename);
return;
}
}
const getConstantsTypeRegex = () => /-\s*\((?<type>.*)\)getConstants\s*{/;
const getConstantsTypeRegexMatch = getConstantsTypeRegex().exec(code);
if (!getConstantsTypeRegexMatch) {
throw new Error(
`Failed to parse return type of getConstants method in file ${relFilename}`
);
}
const getConstantsType = getConstantsTypeRegexMatch.groups.type;
const getConstantsBody = code
.split(getConstantsTypeRegex())[2]
.split("\n}")[0];
const newGetConstantsBody = `
__block ${getConstantsType} constants;
RCTUnsafeExecuteOnMainQueueSync(^{${getConstantsBody
.replace(/\n/g, "\n ")
.replace(/_bridge/g, "self->_bridge")
.replace(/return /g, "constants = ")}
});
return constants;
`;
writeFile(
filename,
code
.replace(getConstantsBody, newGetConstantsBody)
.replace("#import", "#import <React/RCTUtils.h>\n#import")
);
});
console.log("Files without constantsToExport: ");
filesWithoutConstantsToExport.forEach((file) => console.log(file));
console.log();
console.log("Files with constantsToExport but no getConstants: ");
filesWithConstantsToExportButNotGetConstants.forEach((file) =>
console.log(file)
);
console.log();
console.log("Files with requiresMainQueueSetup = NO: ");
filesExplicitlyNotRequiringMainQueueSetup.forEach((file) =>
console.log(file)
);
}
if (!module.parent) {
main();
}
```
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D21797048
fbshipit-source-id: a822a858fecdbe976e6197f8339e509dc7cd917f
Summary:
These logs are no longer necessary, because data indicates that the TurboModule eager init crash was fixed.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D21852743
fbshipit-source-id: ddeefd6396283ee5e15980a33fb006cb83a81532
Summary:
I was doing some bundle inspection and noticed the LogBox module was included. It does amount to around 15kb so I think it is worth making sure it is not there.
To fix it I moved some imports to require inside __DEV__ blocks to make sure metro is able to remove these imports.
## Changelog
[General] [Fixed] - Make sure LogBox is not included in production bundles
Pull Request resolved: https://github.com/facebook/react-native/pull/28984
Test Plan: Tested using react-native-bundle-visualizer and made sure nothing from LogBox was included in the bundle after these changes.
Reviewed By: TheSavior
Differential Revision: D21794466
Pulled By: rickhanlonii
fbshipit-source-id: 6cb0c0a89633e9850019bd61478c35e9c21638dc
Summary:
Remove usage of the legacy context API in Modal in favor of the new context API. Closes https://github.com/facebook/react-native/issues/28103
Pull Request resolved: https://github.com/facebook/react-native/pull/29002
Test Plan: Run RNTester app and test the Modal example. Also add a `console.warn` to make sure we get the correct context value.
Differential Revision: D21793993
Pulled By: TheSavior
fbshipit-source-id: de2a30cbd46507bfa73a563d2429c5a7f0e320c9
Summary:
Changelog: [Internal]
# Problem
JS inspector overlay gets flattened, therefore `onResponderMove` callback isn't called. JS inspector depends on `onResponderMove` because that's how it knows where on screen user has tapped.
https://our.intern.facebook.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/Libraries/Inspector/InspectorOverlay.js?commit=2e9ab04b039a&lines=58
# Workaround
This is just a hack to get it working, setting nativeID forces stacking context to be created and therefore view not being flattened.
Reviewed By: mdvacca
Differential Revision: D21816513
fbshipit-source-id: 83aedbfaecdce22cfe202241cbe91cecb914ed6b
Summary:
At some early stages of Instance initialization, it does not have a `nativeToJsBridge_`. At the same time, `handleMemoryPressure` can be called at any point in time, so we should check the pointer for not being null before calling on it.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D21798522
fbshipit-source-id: 6384da88784cceb493cf9810408cbb47777d3f4b
Summary:
This change is especially important for Fabric when a lot of objects (mostly ShadowNodes) have shared ownership. Without this change, JSVM could not know that bunch of natively allocated objects should be deallocated.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D21798527
fbshipit-source-id: f2051721b074b99660cdfd6c5b75679b9792403e
Summary:
Removing logs related to T62192299 that we don't need anymore.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21773629
fbshipit-source-id: d16c01f87be3ed7512fe90b6e261b4c7efbd3068
Summary:
just a minor change - uses the [count api](https://reactjs.org/docs/react-api.html#reactchildrencount)
> Returns the total number of components in children, equal to the number of times that a callback passed to map or forEach would be invoked.
## Changelog
not needed I think
Pull Request resolved: https://github.com/facebook/react-native/pull/28991
Test Plan: tested locally
Differential Revision: D21794081
Pulled By: TheSavior
fbshipit-source-id: bf6d11b2bc854d938aed7268911f89a00bb3f596
Summary:
Changelog: [Internal]
# Problem
Before every update, restoreDefaults is called on animated nodes. In paper this makes sure no stale properties are on animated nodes. In paper it works fine because restoreDefaults is called before mounting and animations are triggered after mounting within single commit.
Details: https://github.com/facebook/react-native/pull/11819
In Fabric however it is called outside of other mounting operations and it applies default values to the view and then re-applies animated values.
Reviewed By: JoshuaGross
Differential Revision: D21786765
fbshipit-source-id: a2cb6d6d9cbd39d4c403c97c2f51e7d92078102f
Summary:
All ReactViewPager functionality should now be possible by using a ScrollView.
Changelog: [Internal] ReactViewPager has been deprecated on Android and was removed from core as part of LeanCore
Reviewed By: mdvacca
Differential Revision: D21751774
fbshipit-source-id: 292475b9ffe7788b745329d13fd88dc3b613819e
Summary:
After the experiment in D21198302 ships to 100% and that code is deleted, we no longer need ViewPagerAndroid anywhere in FB so it can be deleted.
This is distinct from deleting the native side of the component, which resides in open-source.
Changelog: [Internal]
Differential Revision: D21211134
fbshipit-source-id: 610e09792b079d34acbfcec836fde58a0b3648a7
Summary:
This diff reverts the workaround added as part of D21432793, now that D21432793 is released in production for FB4A / FBiOS
changelog:
[Internal]
Reviewed By: JoshuaGross
Differential Revision: D21772129
fbshipit-source-id: 3b39738cb8fc871ec85d87f347c7f0ef408ccc3e
Summary:
This diff extends the measurement of Text components in order to support empty strings.
This is required for parity with Paper.
I created a follow up task to analyze support of empty string as part of the Text infrastructure of Fabric in the future.
changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D21761171
fbshipit-source-id: d2aa074052b09732af5d35723f19014090fcabbf
Summary:
`getStackTracesTreeNodeForAlloc` only works for memory that was tracked with `newAlloc`.
For now, that is only memory that goes through GC APIs.
Various places were calling this function on native memory which was allocated
with `malloc`, not GC memory. This led to SIGSEGV on nullptr when trying to take heap profiles
of these objects.
`newAlloc` could, in theory, work with native memory as well. But building out support for that
is outside the scope of this fix.
Changelog: [Internal]
Reviewed By: jbower-fb
Differential Revision: D21667842
fbshipit-source-id: 8403a6668e5ec607972ce6819f78fedb89da3f37
Summary:
This diff updates our RCTKeyCommands code to be more resilient by copying the [FLEX strategy for key commands](https://github.com/Flipboard/FLEX/blob/master/Classes/Utility/Keyboard/FLEXKeyboardShortcutManager.m).
This strategy swizzles UIApplication handleKeyUIEvent which is further upstream than our UIResponder. It also allows for single key hotkeys like pressing just `r` instead of `cmd+r`. It does this without interfering with typing input by checking the first responder first.
I've also updated our hotkey handling to support using just the keys like `r` in addition to `cmd+r`. In addition to brining these hotkeys more in line with other iOS tools, they're also easier to use and do not suffer the same issues hotkeys with modifiers like `cmd` have where keys are dropped.
Changelog: [iOS] [Added] Allow hotkeys to be used without command key
Reviewed By: shergin
Differential Revision: D21635129
fbshipit-source-id: 36e0210a62b1f310473e152e8305165024cd338b
Summary:
As a followup to D21750097 and D21735940, it seems that ANY uses of childCount will be incorrect as they are "often" reported, incorrectly, as just 0.
This is likely due to exotic cases like (1) the ViewManager childCount being overridden, or (2) special ViewManagers like BottomSheet where the childCount may actually be 1/0 even though many children are inserted.
Anyway, as a more generic fix, let's only rethrow an existing Exception with additional diagnostics instead of trying to detect when this /would/ crash.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21756178
fbshipit-source-id: 17ffb2ed531978bae8d4db19d7b87ec62397e44b
Summary:
We cannot call `parentView.getChildCount()` directly, we must get the child count through the ViewManager:
A simple `Log.e` call shows:
```
MountingManager: parentView.getChildCount(): 0 // viewGroupManager.getChildCount(parentView): 7
```
This difference does not occur for ALL views, but it occurs for... enough that this will crash on basically ~every screen, at least on navigation when all views are removed.
Theory about why this is happening: some ViewGroup types compute childCount differently, especially if we do some sort of custom child management for some view type? By coercing to `T` which the ViewManager does, we call getChildCount on the correct class type. This is just a hypothesis, though. But the failing views are all `View`s and it does look like `ReactClippingViewManager` has custom `getChildCount` logic, so that's likely the answer.
There's no such thing as an easy diff!
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21750097
fbshipit-source-id: 3d87d8f629a0c12101658050e57e09242dfc2a8c
Summary:
When a property returns an array type, use the actual element type when generating structs and inlines.
Changelog: [Internal]
Reviewed By: TheSavior
Differential Revision: D21651351
fbshipit-source-id: 14cadf209c38a301c9c65fcaadd8a292c1936349
Summary:
I have a theory that T53114059 can be caused by (1) other crashes, and/or (2) reloads. Clear out the ID of a RootView when it is detached.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21732066
fbshipit-source-id: 5e607f34cf42ca26bdf199d5d3024fd49a60dc1e
Summary:
This changes how we access the EventDispatcher from the FabricUIManager in bridgeless mode.
Currently, we have implemented a similar API to what we use for Fabric (used in UIManagerHelper): `BridgelessReactContext.getJSIModule(UIManager).getEventDispatcher()`. However, `getJSIModule` does not have a nullable return type, which means that we have to throw an exception if the UIManager can't be found - if, for example, the instance is not initialized yet (or has been destroyed). This is causing crashes when a view tries to access the EventDispatcher before the instance is initialized, which takes longer for Venice because we include JS bundle loading as part of initialization (we may need to revisit that).
Ideally, we'd like to create a first-class API for `getEventDispatcher()`, and make sure that it never crashes if the instance is destroyed, because we don't care if JS events aren't delivered at that point. However, there are some obstacles to making this change for the bridge - mostly related to avoiding circular dependencies between the bridge module and the uimanager module. (Also, this might be a behavior change for the bridge, because I think we currently start queueing events before it's initialized? and product code might be relying on that).
Reviewed By: mdvacca
Differential Revision: D21672949
fbshipit-source-id: a38e96cd40c6f70124b7ca2a5c9722988fe7fcf4
Summary:
See https://github.com/babel/babel/releases/tag/v7.10.0 for changes
It includes a change I asked the Babel team for as they have been wrapping object spread calls with lots of extends call for spec compliance, but that only matters when you have side-effectful getters that affect the rest of the objects in the spread call and are order dependent. We should not be depending on such behavior in FB code so we can use loose mode and benefit from the app size wins.
Changelog: [Internal]
Reviewed By: motiz88
Differential Revision: D21738344
fbshipit-source-id: 5eeff47481e474bb41183119d990692b31751dd5
Summary:
Error messages like P131885276 don't help debugging, even when all mount instructions are logged to logcat. This log would help identify the incorrect mount instruction.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21735940
fbshipit-source-id: 16ff315c11ccafdd75d0ad9c7c60b7be2cd73202
Summary:
This was added in D3343907 June 1st 2016, disabled in D3428043 June 16, 2016 and never re-enabled.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D21635227
fbshipit-source-id: db51dfb6271359bea7da34b4e2a71931fc7c2a63
Summary:
This diff adds a new swizzling method for replacing instance methods with blocks.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D21635131
fbshipit-source-id: c8061817bed66dad160efffee5a13c8714134540
Summary:
Index adjustment doesn't work if virtual views are inserted, because those don't actually result in the view hierarchy being mutated, as such.
Add an Android-specific check to solve the crash.
I don't love adding platform-specific checks here, so I'm considering wrapping this logic in a platform-specific class, so that logic lives outside of the core of LayoutAnimations and entirely in platform code.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D21727805
fbshipit-source-id: 5af2cf479beaa4d0e9d94ea16ac989c4268920f8
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/28983
Creating an interface for EventDispatcher so I can implement it from a dummy/no-op EventDispatcher to use in bridgeless mode when the instance isn't alive.
Changelog: [Android] [Changed] Renamed EventDispatcher to EventDispatcherImpl and created EventDispatcher interface; calls to EventDispatcher contstructor need to be updated
Reviewed By: makovkastar
Differential Revision: D21695401
fbshipit-source-id: 46066a467efcf03a5f484bb9fb58c662d46a2c4e
