Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/34801
D39796598 (https://github.com/facebook/react-native/commit/8cdc9e7f04e2dd3026d15dcd5765952bfb2c5c08) broke this CircleCI job, causing failures for `dtslint` and `prettier`. This fixes both issues, by upating formatting, and removing types from `legacy-properties.d.ts` that we no longer export.
These were not flagged by sandcastle. It looks like this is because:
1. We run eslint, but not dtslint internally
2. Arcanist will ignore anything under a "vendor" directory, while we do not exlude anything from the prettier check in OSS.
I also updated the eslint config to lint any TypeScript that appears outside the types directory, since typings are now spread out.
Changelog:
[Internal][Fixed] - Fix `analyze_code` CircleCI Job
Reviewed By: cipolleschi
Differential Revision: D39848604
fbshipit-source-id: 844dfe26e4b618059542b29df163402079c39322
Summary:
This adds the `alt` prop to the `Image` component as requested on https://github.com/facebook/react-native/issues/34424. Using this new `alt` prop enables the `accessibility` prop and passes down the alt text to `accessibilityLabel`. This PR also updates RNTester ImageExample in order to facilitate the manual QA.
#### Open questions
- ~~On web `alt` text is displayed on the page if the image can't be loaded for some reason, should we implement this same behavior if the `Image` component fails to load `source`?~~ Not for now
## Changelog
[General] [Added] - Add alt prop to Image component
Pull Request resolved: https://github.com/facebook/react-native/pull/34550
Test Plan:
1. Open the RNTester app and navigate to the Image page
2. Test the `alt` prop through the `Accessibility Label via alt prop` section, this can be tested either by enabling Voice Over if you're using a real device or through the Accessibility Inspector if you're using a simulator
https://user-images.githubusercontent.com/11707729/187790249-0d851363-c30e-41b6-8c24-73e72467f4ba.mov
Reviewed By: lunaleaps
Differential Revision: D39618453
Pulled By: cipolleschi
fbshipit-source-id: 0e26b2574514e76ce7e98ddb578f587a9cc30ee9
Summary:
Currently, height is sometimes the only valid option for pushing `TextInput` up in the layout on Android. The problem is when switching keyboards. For instance, switching from ABC to emojis. This will trigger keyboard show events and recalculate the height for the `KeyboardAvoidingView`. Since the keyboard is still showing, the view has the height that was previously calculated and thus `frame` represents that. This means the `frame.height` has adjustments for the keyboard calculated in it, but it is used the same way as if the keyboard was not showing. This results in wrong calculation and the input showing at the incorrect place in the layout (mostly hidden under the keyboard)
This fix simply uses the previous calculation to offset `frame.height`, resulting in the correct height and smooth switching between keyboards. It's also scoped only to height mode since that's where the problem shows.
_Note: I mention android here, but it fixes it for both platforms. It's just that iOS usually works best with different behaviour so it's rarely used there._
## Changelog
[General] [Added] - Include `this.state.bottom` when calculating new keyboard height to fix android keyboard switching
Pull Request resolved: https://github.com/facebook/react-native/pull/34749
Test Plan:
With simple code:
```jsx
import { StatusBar } from "expo-status-bar";
import React from "react";
import {
KeyboardAvoidingView,
StyleSheet,
Text,
TextInput,
View,
} from "react-native";
export default function App() {
return (
<KeyboardAvoidingView style={styles.container} behavior="height">
<Text>Open up App.js to start working on your app!</Text>
<StatusBar style="auto" />
<TextInput style={{ backgroundColor: "red", width: "100%" }} />
</KeyboardAvoidingView>
);
}
const styles = StyleSheet.create({
container: {
padding: 32,
flex: 1,
backgroundColor: "#fff",
alignItems: "center",
justifyContent: "space-between",
},
});
```
Notice the consistency of the TextInput after the changes, while before it would just move around more you switch the keyboards.
| Before | After |
|---|---|
|  |  |
Reviewed By: cipolleschi
Differential Revision: D39718812
Pulled By: NickGerleman
fbshipit-source-id: 2550182e846f3f8e719d727fa8e6d87165faebf6
Summary:
React Native's TS definitions are currently mostly stored in one monolithic file. This change splits the definitions up to correspond to the source files they came from, and are placed next to the source files. I think this should help inform, and make it easy to update the TS declarations when touching the Flow file.
I noticed as part of the change that the typings have not yet removed many APIs that were removed from RN. This is bad, since it means using the removed/non-functional API doesn't cause typechecker errors. Locating typings next to source should prevent that from being able to happen.
The organization here means individual TS declarations can declare what will be in the RN entrypoint, which is a little confusing. Seems like a good potential next refactor, beyond the literal translation I did.
Changelog:
[General][Changed] - Place TS Declarations Alongside Source Files
Reviewed By: lunaleaps, rshest
Differential Revision: D39796598
fbshipit-source-id: b36366466fd1976bdd2d4c8f7a4104a33c457a07
Summary:
There was a bug on Android when an Animated.ScrollView had a RefreshControl while an Animated style was applied, ie `transform`:
```
<Animated.ScrollView
refreshControl={<RefreshControl />}
style={{
transform: [{
translateY: new Animated.Value(200, {useNativeDriver: true})
}]
}}
/>
```
The transform value was being incorrectly applied twice. Since the styles were applied once on RefreshControl and once on NativeScrollView, the transform style is effectively applied twice:
**1. ScrollView.js**
- RefreshControl gets the transform through Fabric commit
- [The RefreshControl gets wrapped around ScrollView](https://fburl.com/code/k60krxbj) while on iOS there is no change in the parent/child relationship. [Outer/inner styles are split and applied to RefreshControl/ScrollView](https://fburl.com/code/b2to75er), and transform styles are applied on the parent (RefreshControl)
**2. createAnimatedComponent.js**
- NativeScrollView gets the transform through Animated
- [ScrollView forwards its ref to NativeScrollView](https://fburl.com/code/w1whtl5f), which means AnimatedComponent is setting the transform styles on NativeScrollView and not RefreshControl as ScrollView.js did
This diff fixes this bug by using the `useAnimatedProps` hook which makes both RefreshControl and ScrollView components into animated components. Otherwise, the components don't know what to do with Animated values.
---
Changelog:
[Internal][Fixed] - Animated transform style properties were being applied twice when used on an Animated.ScrollView with RefreshControl on Android
Reviewed By: javache
Differential Revision: D38815633
fbshipit-source-id: 2b76639d2237176b6aae4fb1e22cf1a1ec70a69a
Summary:
Currently, it is possible in one of the "minimum press duration" unit tests for certain instructions to take longer than expected, skewing the return value of `Date.now()` by at least 10ms.
This changes the unit test to mock `Date.now()` more accurately so that the test is no longer flakey.
Changelog: [Internal]
Reviewed By: jacdebug
Differential Revision: D39804152
fbshipit-source-id: ab62fd1921bd015d969da9595bd3267c38c6e59c
Summary:
Currently, fbios link groups depend on supermodules to generate. To continue supporting
link groups while allowing us to migrate off supermodules, we'll add a `fbios_link_group` label on all fbios targets to persist the current hierarchical information.
Reviewed By: jkeljo
Differential Revision: D39543862
fbshipit-source-id: 2abe5b3ee883e77336076d88436a1c84139b5a97
Summary:
This updates the `transform` property to support string values as requested on https://github.com/facebook/react-native/issues/34425. This also updates the existing unit tests of the `processTransform` function ensuring the style processing works as expected and updates the TransformExample on RNTester in order to facilitate the manual QA of this.
## Changelog
[General] [Added] - Add string support to the transform property
Pull Request resolved: https://github.com/facebook/react-native/pull/34660
Test Plan:
1. Open the RNTester app and navigate to the Transforms page
2. Check the transform style through the `Transform using a string` section
https://user-images.githubusercontent.com/11707729/189550548-ee3c14dd-11c6-4fd1-bd74-f6b52ecb9eae.mov
Reviewed By: lunaleaps
Differential Revision: D39423409
Pulled By: cipolleschi
fbshipit-source-id: 0d7b79178eb33f34ae55a070ce094360b544361f
Summary:
when specifying dev server without port, e.g. http://www.example.com/, there are some issues.
1. redbox error
<img src="https://user-images.githubusercontent.com/46429/190540390-8ee420f2-7642-427b-9f2e-e0c6d31015f8.png" width="30%">
2. showing -1 in loading view
<img src="https://user-images.githubusercontent.com/46429/190540727-158f35ad-359f-443a-a4b0-768dd2f7e400.png" width="50%">
the root cause is coming from [`java.net.URL.getPort()` will return -1 when the url doesn't have a port](https://developer.android.com/reference/java/net/URL#getPort()). this pr replaces the parser to [`okhttp3.HttpUrl`](https://square.github.io/okhttp/4.x/okhttp/okhttp3/-http-url/#port) that it will have default port 80 for http or port 443 for https. the two call paths should only serve http/https address, not file:// address. it should be safe to change from java.net.URL to okhttp3.HttpUrl.
not fully related, in the case above, android will connect to `ws://www.example.com/:8097` for react-devtools
we should strip the trailing slash in *setUpReactDevTools.js*
## Changelog
[Android] [Fixed] - Fix port as -1 if dev server without specifying port on Android
Pull Request resolved: https://github.com/facebook/react-native/pull/34705
Test Plan:
test on rn-tester with the following steps
1. `yarn start`
2. open another terminal and run `ngrok http 8081` and it will return a tunnel url, e.g. `71a1-114-36-194-97.jp.ngrok.io`
3. open dev setting in app and change the dev server to `71a1-114-36-194-97.jp.ngrok.io`
5. reload the app
Reviewed By: cipolleschi
Differential Revision: D39573988
Pulled By: cortinico
fbshipit-source-id: 397df90ab30533207bd87a3f069132d97c22c7fd
Summary:
## Summary
In the past, NativeAnimatedModule could animate **both** Paper **and** Fabric components.
For Fabric nodes, we needed to manually flush NativeAnimatedModule's operations queue. So, we started tracking which nodes were Fabric owned in NativeAnimatedModule.
## Changes
With bridgeless mode, all components must be Fabric-owned. So, should be able to remove this fabric ownership tracking logic and **always flush.**
## Is this safe?
In the worst case, we over-flush. This doesn't seem bad. cc sammy-SC.
## Do we still need flushing?
Arguably, all this manual flushing should be unnecessary, because we already migrated AnimatedModule's Paper integration to RCTSurfacePresenterObserver, here: D14336760 (https://github.com/facebook/react-native/commit/544d9fb10b5a73bd499feb18dab1a7dc11738748). So, do we still need this flushing?
Yes. Here's what happens when you disable all the manual flushing in bridgeless mode: https://pxl.cl/2dqPf.
Long-term, we need to re-think this operations queuing in NativeAnimatedTurboModule. I left my thoughts in T130668424 (Investigation - Day 5).
Changelog: [Internal]
Reviewed By: p-sun
Differential Revision: D39592477
fbshipit-source-id: e971edc0d99661a37b5f430bce46c78acaa121c0
Summary:
`verifyVirtualizedList` chokes on missing displayName in DEV, due to a mistake in D39648806 (https://github.com/facebook/react-native/commit/a0d1585cdb5ae86e5d4e5bc51fdfa9ae7e049b4b). Not sure why this isn't triggered in Jest tests (maybe it is able to fill displayName via transform?), but this fixes the issue in dev mode.
Changelog:
[Internal][Fixed] - Fix crash in DEV when VirtualizedList_EXPERIMENTAL is loaded
Reviewed By: mdvacca
Differential Revision: D39670991
fbshipit-source-id: b1a784aa3fffdc5f2f025eb1f689307132aad9b6
Summary:
This PR refactors the Animated directory to use ESModule imports/exports instead of using a mixture of the 2 module formats, as requested on https://github.com/facebook/react-native/issues/34425.
## Changelog
[Internal] [Changed] - Convert all files in the Animated directory to use ESModule imports/exports
Pull Request resolved: https://github.com/facebook/react-native/pull/34539
Test Plan: This doesn't really add or modify any existing features so checking if CI passes should be enough
Reviewed By: yungsters
Differential Revision: D39235720
Pulled By: yungsters
fbshipit-source-id: 84b4c0a71dc9fca1ab7053263f1cf7c336df58c1
Summary:
VirtualizedList uses a combination of ES6 and CommonJS imports/exports. This moves fully to ES6 imports and exports, in the experimental version (which should soon replace the original).
Changelog:
[Internal][Changed] - Move VirtualizedList_EXPERIMENTAL to ES6 import/export
Reviewed By: rshest
Differential Revision: D39648806
fbshipit-source-id: a5da40f62a6010cd5b9894b02a91af86d3a8b995
Summary:
VirtualizedList is large and complicated, getting larger and more complicated. This splits out a subcomponent, the cell renderer into its own file, since it is relatively isolated already. This uses the copy from VirtualizedList_EXPERIMENTAL, whose only real difference is exposing focus capture events to the containing VirtualizedList.
Changelog:
[Internal][Changed] - Extract VirtualizedListCellRenderer
Reviewed By: rshest
Differential Revision: D39648087
fbshipit-source-id: bb7c2eff0c658713c256650596f86e8788019baf
Summary:
Now that the prop noops, remove the usages of the property. This is the point of no return for the prop.
Replaced listKey usage with a combination of `flow check`, regex replace, manual inspection.
Changelog:
[General][Removed] - Remove usages of listKey
Reviewed By: fred2028
Differential Revision: D39589089
fbshipit-source-id: 722ebb7dce038a2709656394b7d736e72f488bb7
Summary:
Following up on the previous diff to remove the only usage of `listKey` for persistent association, we can remove the need for a manual `listKey`, and instead rely on per-instance association via refs.
A followup change will remove the existing usages.
Changelog:
[General][Removed] - Remove VirtualizedList `listKey` prop
Reviewed By: p-sun
Differential Revision: D39466677
fbshipit-source-id: 6b49f45c987fff9836918ba833fbb16f24414ff8
Summary:
Keeping the lowercase values but keeping the conversion values the same.
D39478951 (https://github.com/facebook/react-native/commit/ea1ec29ba67c837c82c74357e313bd4505b81eb8) reverted some changes that moved it back to uppercase but needed to add new values to `____FontWeight_Internal`. This diff removes the need for the uppercase values
Changelog: [Internal]
Differential Revision: D39506944
fbshipit-source-id: 079f73c1adcf2db28b961523b05562e21fcf3630
Summary:
This diff updates the documentation for enableCppRenderSystem feature flag
This is a follow up of D38725771 (https://github.com/facebook/react-native/commit/399907fe4ad843ac6aa0aa2f553ba3aee3c83006)
changelog: [internal] internal
Reviewed By: cortinico, makovkastar
Differential Revision: D39558834
fbshipit-source-id: e079ee688a912dfa2670c66e5adf347a902a45e1
Summary: Changelog: [Internal] Add flow declaration for Alert. Couldn't figure out how to do type statics on interfaces in Flow.
Reviewed By: yungsters
Differential Revision: D39186533
fbshipit-source-id: 928ee76080490a133c9773d2330473a688e0612d
Summary:
This diff creates a new Feature Flag to Enable/Disable C++ Render System in JS
Changelog: [Internal] Internal
Reviewed By: sammy-SC
Differential Revision: D38725771
fbshipit-source-id: 3a238781bec427274dbd8f7c045d360135d1803e
Summary:
This task has been created T131775291 that blames D39268920 (https://github.com/facebook/react-native/commit/f1c1f8116ba1cfa9d10c5b8c30b98b796047b9c2). I reverted part of the changes and added the support of capital letters for the font weight. It should make the test pass.
However, I am a bit puzzled: I tried to change the title of the `GemstoneTitleBar` and the test was not showing the changed value. I think I'm missing something.
Changelog: [Internal]
Reviewed By: GijsWeterings
Differential Revision: D39478951
fbshipit-source-id: d3e28336fe52c5c58b49872c3f5b6e63e1e37ca5
Summary:
When `getItemLayout` is not supplied, VirtualizedList doesn't know the measurements for its child items ahead of time. It caches most recent cell measurements so that an item which is virtualized away is associated with its correct height. VirtualizedList will not consume space for an item it has not yet rendered/knows measurements for.
This also means an initially created list will only take up the height of its `initialNumToRender()`. This can cause scroll jumping for the scenario of nested VirtualizedLists of the same orientation, since a virtualized away child list will shrink (compared to the spacer), then re-expand, once returned to.
There is logic in the VirtualizedList constructor to load cached frame metrics based on supplied listKey, to enable reloading most recent state. Though locally it seems to be reading the context too early in the lifecycle (compared to other successful checks during componentDidMount, render). It was moved from `componentDidMount` to `constructor` as part of the initial change (https://www.internalfb.com/diff/D6330846 (https://github.com/facebook/react-native/commit/2668dc8e1be7bf93e65ca2e11c87cbeef3310c3e)?dst_version_fbid=131968984136932&transaction_fbid=1578362385612542), and I think has likely been unreliable or dead since.
Restoring state was the only usage of a durable key for nested lists beyond their lifetime. So removing the dead code lets us remove the need for a `listKey`, which has been error-prone. This still leaves the problem present since inception, but there may be alternate solutions like scroll anchoring.
Changelog:
[Internal][Removed] - Remove dead code for restoring state to nested VirtualizedList
Reviewed By: ryancat
Differential Revision: D39466678
fbshipit-source-id: fc2f39802e5cdf9b920974333be64c74211b99a9
Summary:
This updates `fontVariant` to support space-separated string values, i.e., `'small-caps common-ligatures'`, thus aligning it with the [CSS Fonts Module Level 4](https://drafts.csswg.org/css-fonts/#font-variant-prop) specification as requested on https://github.com/facebook/react-native/issues/34425. This also adds unit tests to the `processFontVariant` function ensuring the style processing works as expected.
## Changelog
[General] [Added] - Add space-separated string support for fontVariant
Pull Request resolved: https://github.com/facebook/react-native/pull/34641
Test Plan:
This can be tested either through `processFontVariant-tests` or by using the following code:
```js
<Text
style={{
fontVariant: 'small-caps common-ligatures',
}} />
```
Reviewed By: javache
Differential Revision: D39423317
Pulled By: cipolleschi
fbshipit-source-id: ad971addb423ed338e178528a11fe9d456c03e6e
Summary:
This aligns the logic of setting style (width / height) and source of Android with iOS.
iOS handles nullish uris with set width and heigth by passing them through. Android did not.
## Changelog
[Android] [Fixed] - Align android image style / source logic with ios
Pull Request resolved: https://github.com/facebook/react-native/pull/34655
Test Plan:
```
<Image source={{width: 100, height: 100}} />
```
Before this Patch:
* iOS: Renders a blank image with 100x100
* Android: Renders a blank image with 0x0
After this Patch:
* iOS: Renders a blank image with 100x100
* Android: Renders a blank image with 100x100
Reviewed By: sammy-SC
Differential Revision: D39423391
Pulled By: cipolleschi
fbshipit-source-id: 997c06dea42e9b69fda12b678a1b82ad8319537f
Summary:
Changelog:
[General][Fixed] In RN devtools, call that method, and then set component state to not inspecting.
Reviewed By: lunaruan
Differential Revision: D38852009
fbshipit-source-id: d14bd4cc4461f0f7ec4ae9cd6098a58eef3e6ab9
Summary:
Changelog:
[General][Fixed] - Currently selectNode doesn't work for Fabric. Passing the instance instead of a tag to `selectNode` works.
Reviewed By: lunaruan
Differential Revision: D38851141
fbshipit-source-id: 7640d0f31fb099346a4d8205981e262da6be4990
Summary:
Jest 27 [flips defaults](https://jestjs.io/blog/2021/05/25/jest-27#flipping-defaults), making "modern" fake timers (ie, `sinonjs/fake-timers`) the default, over "legacy", which we currently use.
We're not quite ready for modern timers yet, partly due to Promise polyfills (https://github.com/facebook/react-native/pull/34659), and also some internal issues with specific tests.
This makes our use of legacy timers explicit in preparation for an update to Jest. A switch to modern timers may follow later.
Changelog:
[Internal][Changed] - Prepare for Jest 27+ by making legacy fake timer use explicit
Reviewed By: jacdebug
Differential Revision: D39383719
fbshipit-source-id: 6a3dc6f8547cc76f7702a5a39c9b30a184303f17
Summary:
This test currently uses `jest.runAllTicks()` to execute cached, immediately-resolving promises, under Jest "legacy" timers (the RN default) - this works only because [we polyfill Promise](https://github.com/facebook/react-native/blob/main/jest/setup.js#L24) using a userland JavaScript implementation that internally uses mocked-out timer functions.
Here we change to a more universal approach by adding a new microtask/promise to the end of the queue and awaiting it.
This allows us to remove our Promise polyfill from Jest setup (to follow).
Changelog:
[Internal][Fixed] - Prepare `LogBoxLog-test.js` for native promises
Reviewed By: huntie
Differential Revision: D39418413
fbshipit-source-id: 1384ef385b1e10261754513369af8997d296ffea
Summary:
This test was causing uncaught promise rejections, because the mocked `symbolicateStackTrace` in this test returns a different type from the real one, causing `LogBoxSymbolication`'s unawaited, cached promises to reject.
This wasn't picked up as a test failure because our `promise` polyfill silently swallows uncaught rejections, but it does cause a failure if we use native promises in tests.
Changelog:
[Internal][Fixed] - Fix LogBoxSymbolication-test.js
Reviewed By: huntie
Differential Revision: D39418412
fbshipit-source-id: 0be8f1551c4a58dc47faf1597caf47271af40af2
Summary:
This PR adds support for number values for `fontWeight` as requested in https://github.com/facebook/react-native/issues/34425.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[General] [Added] - Added support for number values in fontWeight.
Pull Request resolved: https://github.com/facebook/react-native/pull/34598
Test Plan:
```js
<Text style={{ fontWeight: 900, color: 'red' }}>
Hello World
</Text>
```
Reviewed By: jacdebug
Differential Revision: D39268920
Pulled By: cipolleschi
fbshipit-source-id: 9bb711677bf173f9904b74f382659042856efd83
Summary:
This adds the `id` prop to `Text`, `TouchableWithoutFeedback` and `View` components as requested on https://github.com/facebook/react-native/issues/34424 mapping the existing `nativeID` prop to `id`. As this components are inherited by others this also adds the `id` prop support to `Image`, `TouchableBounce`, `TouchableHighlight`, `TouchableOpacity` and `TextInput`.
This PR also adds android tests ensuring that the `id` property is passed to the native view via the `nativeID` prop, these tests were based on the existing `nativeID` tests ([NativeIdTestCase.java](https://github.com/facebook/react-native/blob/main/ReactAndroid/src/androidTest/java/com/facebook/react/tests/NativeIdTestCase.java)).
## Changelog
[General] [Added] - Add id prop to Text, TouchableWithoutFeedback and View components
Pull Request resolved: https://github.com/facebook/react-native/pull/34522
Test Plan: Ensure that the new `id` prop android tests pass on CircleCI
Reviewed By: lunaleaps
Differential Revision: D39089639
Pulled By: cipolleschi
fbshipit-source-id: 884fb2461720835ca5048004fa79096dac89c51c
Summary:
Changelog: [internal]
This brings back a fix for ScrollView on iOS that we removed because we thought it was no longer necessary, but it actually is.
Reviewed By: jacdebug
Differential Revision: D39382967
fbshipit-source-id: f052b4fb899f4e60e8c530990caf07344a907c43
Summary:
This adds a a couple of tests to run existing behavior where VirtualizedList avoids batched renders until it has seen scroll metrics when the list is not initialized to the zero position.
Changelog:
[Internal][Added] - Add VirtualizedList tests for handling non-zero initialScrollIndex before scroll metrics
Reviewed By: yungsters
Differential Revision: D39247250
fbshipit-source-id: 530f58f9c7421949c388048c376fc5b0ab7835d9
Summary:
This PR aims to add support for objectFit a partial equivalent to the resizeMode style and prop of Image.
## Changelog
[General] [Added] - Add support for objectFit style of Image.
Pull Request resolved: https://github.com/facebook/react-native/pull/34576
Test Plan:
1. Open the RNTester app and navigate to the Image page
2. See the Object Fit section.
Reviewed By: rickhanlonii
Differential Revision: D39261176
Pulled By: jacdebug
fbshipit-source-id: 1eefd76b6c11ed5fc52b2c524ad78c91051077f6
Summary:
Changelog: [internal]
This replaces all direct references to the `ReactNative` module (which is the Paper renderer) to `RendererProxy` which would select between Paper and Fabric correctly.
The implementation of these functions is exactly the same right now.
As per the removal of the fix for T55744311 in `ScrollView`, I verified this doesn't cause any issues in the screen where it failed before.
Reviewed By: javache
Differential Revision: D39270691
fbshipit-source-id: 03882748fe4b754b9a2c5e9d4c4f003b94ed49ef
