WIP: Flow props

[borisyankov]

Good idea.
Also likely needed for RN 0.59 upgrade.

[gnprice]

Hmm interesting! Can you say more about why this is likely needed for the upgrade?

(Typical example, for the sake of making this thread relatively self-contained to understand:

-type Props = {|
+type OwnProps = {|
   label: string,
   href: string,
+|};
+
+type StateProps = {|
   realm: string,
 |};
 
+type Props = {|
+  ...OwnProps,
+  ...StateProps,
+|};

The branch makes similar changes in a broad sweep across our codebase.)

[gnprice]

OK, I just spent a bit of time looking at the Flow aspect of #3422 (the RN upgrade), and I think I perhaps have some idea what led to this. At the tip of the branch, there are 188 errors, and 130 of them look like either this:

Missing type annotation for CP. CP is a type parameter declared in function
type [1] and was implicitly instantiated at call of connect [2].

     src/common/OfflineNotice.js
      51│   }
      52│ }
      53│
 [2]  54│ export default connect((state: GlobalState) => ({
      55│   isOnline: getSession(state).isOnline,
      56│ }))(OfflineNotice);
      57│

or similar with s/CP/SP/g.

I noticed a while ago that the type for connect in the flow-typed libdef is just wrong. To date the consequence has been that we don't get much useful type-checking around it, particularly to match up its argument (and therefore the types of selectors) with the props type of the wrapped component. One project I've been meaning to return to and finish is to fix that -- see gnprice@524559c66 for previous work I did on that.

This looks like a new aspect of the same problem, with I guess new urgency. How about I take this one? It'll be made easier by the previous investigations I've done into this rather gnarly (and yet not even right) type.

[gnprice]

At the tip of the branch, there are 188 errors, and 130 of them look like

BTW a command I found handy for getting this kind of summary:

node_modules/.bin/flow --json | jq '.errors|map(.message[0].descr)|.[]' -r | sort | uniq -c

This runs Flow in JSON-output mode, and uses jq to pull out the relevant bits of the information.

[borisyankov]

The Flow errors indeed are caused by Flow 0.89 and not React Native.

https://github.com/facebook/flow/blob/master/Changelog.md

Likely to cause new Flow errors:

Big revamp to React typing with the goal of adding support for React.forwardRef and better typing higher-order components. Docs are available here:
https://flow.org/en/docs/react/hoc/

[borisyankov]

Actually, it was Flow 0.85 and this is more information:
https://medium.com/flow-type/asking-for-required-annotations-64d4f9c1edf8

[gnprice]

Ah excellent -- thanks for tracking down and posting those links!

[gnprice]

Scanning through that changelog now. This in 0.84 is one that had been bugging me, and I'm happy to see is fixed:

Likely to cause new Flow errors:

• When a variable is equality-checked with a literal, the variable's type is refined. Earlier, if the variable's type was incompatible with the literal's type, it would silently be refined to empty, whereas now this is an error.

That's the fix that surfaced the error fixed in yesterday's 0e8e9d0 "recipient: Cut unreachable nonsense outbox case." More generally, it's the problem I complained about in the commit message of e6868d8 :

[...] when we say

  switch (action.type) {
    case NOT_FOO_LOL:
      // ... use `action` ...
      break;

    // ...
  }

with a `case` statement for a value of `type` that we've claimed is
*impossible*... Flow quietly just decides that `action` has type
`empty` and there's nothing to worry about.

Then if our code goes and passes that `action` value to *any function
at all*, Flow considers it valid.  After all, there are no values of
type `empty` (that's what `empty` is all about)... so it's true that
every value of type `empty` is a valid value of whatever type it is
that the function expected.

So, that's pretty unfortunate. [...]

[gnprice]

Oh, and this (in Flow 0.88) is very interesting:

• Made Function and Object types be aliases for any. They were always unsafe types, just like any, but they had peculiar behavior. This change revealed places where they were handled improperly within Flow, and ended up surfacing type errors that were previously missed.

They were always basically just as bad as any; making them literally exactly the same thing will make it simpler to talk about 🙂

[gnprice]

I did a quick survey of which Flow version causes which error.

Experimental method:

• Upgrade to a given version of Flow, run Flow, look at errors:
  git reset --hard && perl -i -0pe 's/0\.78\.0/0.84.0/g' package.json .flowconfig && yarn && yarn flow
• Ignore errors in RN or other library code; consider only errors in our code.

---

Results:

(from master, 96f285f -- so this doesn't include the handful fixed yesterday)

Flow 0.80

One error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/start/AuthScreen.js:38:11

Sketchy null check on string [1] which is potentially an empty string. Perhaps you
meant to check for null or undefined [2]? (sketchy-null-string)

        src/start/AuthScreen.js
        35│   componentDidMount = () => {
        36│     Linking.addEventListener('url', this.endOAuth);
        37│     Linking.getInitialURL().then(initialUrl => {
        38│       if (initialUrl) {
        39│         this.endOAuth({ url: initialUrl });
        40│       }
        41│     });

        node_modules/react-native/Libraries/Linking/Linking.js
 [2][1] 80│   getInitialURL(): Promise<?string> {

This is the only error added up through Flow 0.84.

Flow 0.85

• Those 130 connect errors.

  (I then adjusted the react-redux libdef to say declare export var connect: any, to suppress those so I could see the rest.)

• Two other errors that look like the same pattern:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/settings/LanguagePicker.js:29:5

Missing type annotation for U. U is a type parameter declared in function type [1]
and was implicitly instantiated at call of method map [2].

[...]

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/styles/theme.js:41:29

Missing type annotation for T. T is a type parameter declared in function type [1]
and was implicitly instantiated at call of method createContext [2].

     src/styles/theme.js
      38│ };
      39│ themeColors.default = themeColors.light;
      40│
 [2]  41│ export const ThemeContext = React.createContext(themeColors.default);

This covers through 0.88.

Flow 0.89

One more:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/boot/TranslationProvider.js:28:24

Property _ is missing in object type [1] but exists in object type [2].

     25│  */
     26│ export function withGetText<P: { _: GetText }, C: ComponentType<P>>(
     27│   WrappedComponent: C,
 [1] 28│ ): ComponentType<$Diff<ElementConfig<C>, { _: GetText }>> {
 [2] 29│   return class extends React.Component<ElementConfig<C>> {
     30│     render() {
     31│       return (

... And that's it, all the way to Flow 0.92!

[gnprice]

The error surfaced in 0.89 is probably the React HOC change you linked to above.

The one in 0.80, I don't see anything in the changelog that looks like an explanation of why it showed up then... but also the actual error is straightforward to understand and fix, so shrug.

[gnprice]

I think this is now superseded by #3450 and the subsequent e7ee1c9, so closing.

For cross-reference to help find this discussion later if desired: this was part of #3399 .

[borisyankov]

Hmm, I don't agree.
The purpose of this was not only to allow Flow not to break but to also make it clear where each property came from.

[gnprice]

Ah, OK then! Let's discuss that. I definitely think there's room to improve in that direction.

Would you send a new PR for that? This thread has quite a bit of history already, which is almost all focused on the Flow situation, so I think it'll be easier to discuss in a new one anyway.

Quick first thoughts, based on this version:

• There'll need to be a bit more explanation of what this is doing and why. This was marked as WIP, so perhaps you knew that.
• There's definitely some cost here -- the net diffstat is nearly +500 lines, and that's largely made up of code that feels kind of boilerplatey:

+type Props = {|
+  ...OwnProps,
+  ...StateProps,
+|};

That doesn't mean it can't be worth it, but it means some discussion is needed of why it is. Also it'd be good to try to find variations that are less boilerplate.