Fix Flow types (and update RN version) #2619
Conversation
…ative libraries Currently, there are numerous ignored libraries that are hiding type errors. Actually, they're causing type errors too. This sort of thing only patches over actual problems, so we have to revert them to get a config we can build upon.
We want to have Flow types working with the latest packages.
The current typing is clearly a typo, as it is circular. `NavigationScreenProp` should be used to type the navigation prop
The current code thinks it can import these, but this isn't true, and was being hidden because the .flowconfig ignored the whole react-native package. There's no easy to type Text and View at the current moment, as far as I can tell. Importing the highly generic `StyleObj` seems like the best bet, and is what I have being using in my projects.
There was a problem hiding this comment.
Well looks like you are doing a great job of iliminating some of the ignoring issues and ignoring creating issues-issues keep up the good. From what i can see this PR should benefit us reducing flow errors so with this review I will close this issue. But if i have missed something dont be afraid to reopen it. :)
…eact-navigation.js
|
Hey @mrm1ck, I'm not really sure what you're talking about, and your PR doesn't really explain it. What test failures are you addressing? Is the new folder you're creating ( Sorry if I'm missing something obvious; I'm not too failure with how React Navigation tests. As far as I can tell, however, the CircleCI failures are all the result of Prettier Lint complaints that don't have anything to do with this PR (ie. Lint is failing on the entire project right now due to Prettier). @skovhus - Good find. Right now there is a I went ahead and removed that file (as well as |
|
@grabbou, in 93976d3 ("Introducing flat options") you updated The type errors with the website code didn't get uncovered until now, since the Flow configuration on this project has been broken for quite a while. I am trying to fix it. Can you provide some guidance on what we should do here? Are the new parameters optional? If not, how should we update the website code? Should the docs still mention For now, I'll proceed under the assumption that the new parameters were supposed to be typed as optional, as this would keep the functionality in the docs/website still working, and everything typechecks with them as optional. |
…eateNavigator optional
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * @noflow - get/set properties not yet supported by flow. also `...require(x)` is broken #6560135 | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
Okay, so I went ahead and integrated this version of the library into my personal app. I got a whole bunch of new Flow errors, which was a good indication that things were working! I worked through all them, and I identified one typing issue and one unfortunate shortcoming of Flow. Typing Issue The issue has to do with the Things seem to work okay if I just set these two other props ( If things typecheck, I guess we're good. But for all of these cases, it'd be great if somebody could elucidate why these props were originally listed as required. @grabbou - three questions currently for you, about type errors that have been exposed after fixing up Flow configuration:
Flow Shortcoming Unfortunately, it seems impossible to properly type your exact |
|
I can confirm that the latest version of this PR typechecks both in the root directory, the NavigationPlayground example, and in my personal app. The only possibly sketchy parts are the three questions addressed to @grabbou above. It's worth noting that if ReduxExample was Flow-typed, we would've noticed the "Typing Issue" in my last comment earlier. As it stands, I only noticed it when integrating with my personal app, which means there's no real way to detect when |
|
I'll review this and get it merged over the weekend 😄 |
| duration?: number, | ||
| // An easing function from `Easing`. | ||
| easing?: (t?: number) => number, | ||
| easing?: (t: number) => number, |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
@Ashoat Can you run |
|
Went ahead and ran $ node node_modules/jest/bin/jest.js
FAIL ./App.test.js
● Test suite failed to run
TypeError: Cannot read property 'BLOB_URI_SCHEME' of undefined
at Object.<anonymous> (node_modules/react-native/Libraries/Blob/URL.js:21:21)
at node_modules/react-native/Libraries/Core/InitializeCore.js:174:40
at getValue (node_modules/react-native/Libraries/Utilities/defineLazyObjectProperty.js:44:10)
at ContextifyScript.Script.runInContext (vm.js:53:29)
Test Suites: 1 failed, 1 total
Tests: 0 total
Snapshots: 0 total
Time: 5.059s
Ran all test suites.
error Command failed with exit code 1.
Exited with code 1 |
|
@Ashoat you might want to take a look at this issue facebook/react-native#15810 for the Edit: This comment facebook/react-native#15810 (comment) fixed the issue in our app. Not sure if you want to include it in this package, but it might be a temporary workaround. |
Codecov Report
@@ Coverage Diff @@
## master #2619 +/- ##
==========================================
- Coverage 36.76% 36.73% -0.03%
==========================================
Files 48 48
Lines 1281 1282 +1
Branches 318 319 +1
==========================================
Hits 471 471
Misses 629 629
- Partials 181 182 +1
Continue to review full report at Codecov.
|
….flowconfig Realized this line I removed in the first commit is necessary when using npm link/yarn link, which is what the CircleCI build does
Some of these params are marked as optional because they have defaults. However, the only place `DrawerViewConfig` is used is as the input the function that then applies the defaults
`NavigationNavigatorProps` is used to type the props of the component that is output by the `StackNavigator`, `TabNavigator`, etc. component factories. This component does not need to have any props specified.
`DrawerNavigator`, just like `TabNavigator` and `StackNavigator`, can be called with just a single argument (ie. omitting the config)
|
The In the process of debugging that I discovered that the test config uses Looks like the tests fully pass now! |
|
Thanks for taking such a good care of this PR! Now that tests are passing I think @matthamil will easily give his 💯 - it looks good to me ;D |
|
@Ashoat Sorry for the late response, but I'm glad you got it fixed. Really great work, and I'm really excited to see the community effort in this project ignited again! |
* Fix .flowconfig to stop ignoring modules and properly include React Native libraries Currently, there are numerous ignored libraries that are hiding type errors. Actually, they're causing type errors too. This sort of thing only patches over actual problems, so we have to revert them to get a config we can build upon. * Update react-native/flow-bin dependencies We want to have Flow types working with the latest packages. * Update flow-typed libraries (auto-generated) * Fix typing of navigation prop used by withNavigation HOC The current typing is clearly a typo, as it is circular. `NavigationScreenProp` should be used to type the navigation prop * Fix typing of easing function What's funny is that I fixed this before in d71ed75. @skevy reintroduced the mistyped function in 9436d03, which didn't trigger any Flow errors because .flowconfig was ignoring the entire react-native package * Correct typing of View and Text style prop The current code thinks it can import these, but this isn't true, and was being hidden because the .flowconfig ignored the whole react-native package. There's no easy to type Text and View at the current moment, as far as I can tell. Importing the highly generic `StyleObj` seems like the best bet, and is what I have being using in my projects. * Import NavigationScreenComponent using full path * Updating yarn.lock files * Get rid of library overrides in flow-typed/react-native.js and flow/react-navigation.js * Add @flow to src/react-navigation.js and make last three params to createNavigator optional * Make screenProps and navigationOptions optional in NavigationNavigatorProps * yarn run format * Readd react-navigation/node_modules ignore to NavigationPlayground's .flowconfig Realized this line I removed in the first commit is necessary when using npm link/yarn link, which is what the CircleCI build does * Make all DrawerViewConfig's params optional Some of these params are marked as optional because they have defaults. However, the only place `DrawerViewConfig` is used is as the input the function that then applies the defaults * Make all props in NavigationNavigatorProps optional `NavigationNavigatorProps` is used to type the props of the component that is output by the `StackNavigator`, `TabNavigator`, etc. component factories. This component does not need to have any props specified. * Make second param to `DrawerNavigator` factory optional `DrawerNavigator`, just like `TabNavigator` and `StackNavigator`, can be called with just a single argument (ie. omitting the config) * Upgrade to RN 0.48.4 to address facebook/react-native#15810
7 participants
The main reason there are errors on a stock RN app with react-navigation is that the
.flowconfighad[ignore]s that caused the types to drift from stock packages. The first commit in this PR fixes.flowconfig, the second and third update package and type library versions, and the remaining commits fix the resultant Flow errors.