Resolve react flow definitions

[corbt]

Currently, we're not taking advantage of Flow's built-in type definitions for the React library in all cases because Flow's definition uses declare module react and this file uses import('React'), which Flow thinks is a different library. After this change, the following starts working which didn't before:

import { Component } from 'react-native';

class MyText extends Component<void, {text: string}, void> {
  render() { return <Text>{this.props.text}</Text> }
}

// Correctly throws a Flow error for the missing "text" prop
const renderedText = <MyText />;

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @gabelevi, @janicduplessis and @nicklockwood to be potential reviewers.

[satya164]

Looks reasonable.

[satya164]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1643491035911286/int_phab to review.

[sophiebits]

Shouldn't both names work? https://github.com/facebook/flow/blob/2fdb38c9f31a4a361f3823ef5e901490017810a8/lib/react.js#L181-L185

[corbt]

@spicyj hmm that does look like it should work, but I've tested with fresh RN projects on flow 20.1, and for some reason this class of error is only caught with this change. I haven't done any deeper investigation to see if the react flow interface was loading in RN at all. Since moving from React -> react is on FB's roadmap anyway I figured this was the easiest solution.

[facebook-github-bot]

@corbt updated the pull request.

[facebook-github-bot]

@corbt updated the pull request.

[corbt]

This is still necessary with flow 0.21

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1643491035911286/int_phab to review.

[mkonicek]

Failed with a Flow error that looks unrelated.

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1643491035911286/int_phab to review.

[mkonicek]

I see lots of Flow errors blocking this, e.g.:

Libraries/FBReactKit/js/react-native-github/Libraries/NavigationExperimental/NavigationAnimatedView.js line 140 col 16-21: property `scenes`
Property cannot be accessed on possibly undefined value
Libraries/FBReactKit/js/react-native-github/Libraries/NavigationExperimental/NavigationAnimatedView.js line 140 col 5-14: undefined. Did you forget to declare type parameter `State` of property `Component`?

Looks like Travis shows similar errors? https://travis-ci.org/facebook/react-native/jobs/107038581

[mkonicek]

Removing 'Import failed' because the Flow errors might be legit.

[mkonicek]

@corbt What is Component<void, {text: string}, void>? Why not use propTypes instead? Does Flow look at propTypes?

[corbt]

@mkonicek Flow does look at propTypes. Flow type definitions have three main advantages over propTypes in my experience: 1) they're analyzed statically, so you don't have to wait until the code is run to detect issues, 2) they can express more sophisticated types than propTypes (permitting cool stuff like exhastiveness checks in switch statements), and 3) you can use the same type system anywhere in your program, not just at the boundary between components. Also apparently React is moving towards encouraging Flow over propTypes anyway (facebook/react#1833 (comment)).

The three parameters to Component above are the types of props, state and defaultProps.

It's true that this change generates a ton of Flow errors internally (I count 207), but that's because those components actually do have Flow errors (mostly related to not declaring the type of State on components it looks like), those errors just weren't picked up before because the Flow definitions were broken.

If this PR needs to fix all the internal Flow errors as well I can probably do that, but it's going to be a giant PR. ;)

[mkonicek]

1. Flow type definitions have three main advantages over propTypes in my experience: 1) they're analyzed statically

Can propTypes be analyzed statically?

Don't have enough context to comment on 2 and 3.

From facebook/react#1833 (comment):

Either way, we aren't actively working on the proptypes feature anymore (everything is moving to static analysis tools like Flow), so proptypes are on their way out. I'm not sure if we'd accept a PR, but I don't think this is something we'd actively develop internally at this point.

I'll cc @zpao from React and @jeffmo from Flow here because I don't have much context to review this PR.

Does moving from PropTypes to Flow mean you'd declare your components as extends Component<void, {text: string, anotherProp1: int, anotherProp1: bool, anotherProp3: int, etc. etc.}, void>?

those components actually do have Flow errors (mostly related to not declaring the type of State on components it looks like), those errors just weren't picked up before because the Flow definitions were broken.

I see, that's sad. I think we'd need to update all of those first before landing this unless @jeffmo knows a better way?

[corbt]

I'm happy to fix the flow errors generated by this change if that sounds good to you. It'll probably be a fairly large diff but it's a pretty mechanical process (mostly adding missing definitions) and things will be better typechecked afterwards.

[mkonicek]

I don't really understand this PR well enough unfortunately (see my comments above). Also, many people don't use Flow but rely on propTypes but this PR allows both I think so that should be fine.

Let's wait for @zpao and @jeffmo to comment.

[corbt]

@mkonicek yeah all this PR does is fix Flow's checks, propTypes will continue working as-is (and are detected by flow). The changes I'd have to make would also leave all current propTypes in place, just add definitions to components that are missing propTypes and types on other properties.

[facebook-github-bot]

@corbt updated the pull request.

[corbt]

(btw, the github bot has claimed I've updated this and several other PRs recently that I haven't changed, not sure what's going on with that)

[facebook-github-bot]

@corbt updated the pull request.

[mkonicek]

zpao tells me he has the same comment as spicyj.

[mkonicek]

Is the motivation for this PR to support declaring the props as e.g.:

class MyText extends Component<void, {text: string}, void>

Is it likely people will want to use that syntax?

[corbt]

No, the motivation for this PR is to fix React Native to use the correct
React Flow definitions. The example was just a minimal test case of
something that should work if the correct flow definition of "Component"
were loaded.

I also agree with @spicyj and @zpao that this should already be working
(Flow's built in React definition includes definitions for both "react" and
"React"), but the fact remains that it isn't. I encourage anyone at
Facebook who believes this is already working to create a new RN project
and try the test case I posted in my original PR, and you'll see the flow
definitions aren't right without this PR.

Alternatively, note all the new flow errors created by this PR. If you've
used React with Flow much you'll see that these are legitimate errors
caused by missing type definitions.

On Mon, Feb 22, 2016, 12:25 AM Martin Konicek [email protected]
wrote:

Is the motivation for this PR to support declaring the props as e.g.:

class MyText extends Component<void, {text: string}, void>

Is it likely people will want to use that syntax?

—
Reply to this email directly or view it on GitHub
#5489 (comment)
.

[mkonicek]

Your PR clearly shows that 'React' and 'react' are different things for Flow.

fix React Native to use the correct React Flow definitions

With this PR we get a lot of Flow errors. How would a sample fix of one of them like and what fixing it give us? Sorry for all the back and forth. I don't have enough context yet and the people with the right context haven't been responsive on this PR.

This might be really hard to land with so many Flow errors in open source that have to be fixed and probably many more in internal closed source apps.

[facebook-github-bot]

@corbt updated the pull request.

[corbt]

Ok, I've added another commit to this PR that actually fixes all of the broken flow definitions.

It does so mainly by setting the types of undeclared state and/or props to any, the universal/non-typechecked type. This effectively invalidates the extra typechecking that Flow does for React components' state and props, but it doesn't put us in a worse boat than we were in before because those aren't being checked currently anyway.

I also created a separate commit corbt@faf7cfa that manages to "really" fix flow errors by setting the proper types for state/props where necessary. I spent a couple of hours on this and managed to get down to 126 errors from 215, and with a day's work could have taken care of the rest of them, but I don't have a day to spend on this at the moment. :(

I recommend merging this PR now that all Flow errors are fixed, and then going in and adding the real definitions instead of just any over time to gain more type safety guarantees. The same process will work on your closed source apps: just add any type annotations to anything Flow complains about, then go through later and replace them with real definitions as time permits.

PS enjoy the conference! I'll be watching on the youtube stream, and will understand if it takes a few days to get back on this PR. ;)

[satya164]

@corbt Awesome work. We really need to fix all the Flow errors. Right now it's not usable with Flow at all. I had to add RN to the ignore list in my project :(

LGTM. Need to fix the conflicts though.

[facebook-github-bot]

@corbt updated the pull request.

[corbt]

Conflicts fixed.

[satya164]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[mkonicek]

FYI just saw a related discussion internally:

[satya164]

Actually Flow 0.22 is a lot better at detecting these errors. Never got previous versions of Flow to work with propTypes.