Skip to content

Fix type of connect to fully work, and to work with newer Flow. #3450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 17, 2019

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Apr 11, 2019

This is part of #3399. See also discussion in #3422 and #3428 .

The new RN version (v0.59) comes with a new Flow version (v0.92). That Flow version, in turn, comes with more consistent enforcement of an annotation requirement that (a) on the one hand means fewer bogus "Santa Claus" types ("everything the code wants to be true, is true!", aka empty) but (b) means a lot of boilerplate when using connect, if relying on the libdef for it in flow-typed upstream.

Moreover, the new upstream libdef for connect doesn't understand defaultProps, causing a lot of spurious type errors.

So with this branch, we bring in the new libdef... but then add our own wrapper around it, with an utterly trivial implementation -- if you erase all the types, it looks like this:

export function connect(mapStateToProps) {
  return connectInner(mapStateToProps);
}

but with a bunch of complexity in the types to describe what connect really does, plus about 500 words of comments.

Also marks with a fixme the remaining places where we have type errors hidden by the ineffective old type on connect. I fixed a bunch of such errors last week and the week before, before I realized how much work was needed on the type of connect itself; for the rest, this just marks them so we can move on and fix them separately.


Posting this as a PR because (a) I'd like to shift to more of a practice of posting my own changes for discussion before merge, and (b) this change in particular deserves some discussion, because I want everyone to be aware of it so there are no surprises when Flow complains about a type error on connect in the future.

@gnprice
Copy link
Member Author

gnprice commented Apr 11, 2019

Also filed #3451 for the followup task of fixing the newly-exposed errors.

@gnprice
Copy link
Member Author

gnprice commented Apr 11, 2019

Ah, I forgot there is one type of boilerplate that's still required for the new Flow version. It's rather milder than some of the alternatives, though. Specifically:

  • When mapStateToProps actually takes a props parameter,
  • then its return type needs to be annotated.

For example:

type SelectorProps = {|
  backgroundColor: string,
|};

type Props = {|
  narrow: Narrow,
  dispatch: Dispatch,
  ...SelectorProps,
|};

class ChatNavBar extends PureComponent<Props> {
/* ... */
}

export default connect((state, props): SelectorProps => ({
  backgroundColor: getTitleBackgroundColor(props.narrow)(state),
}))(ChatNavBar);

Even this annotation seems like it shouldn't fundamentally be necessary; but at least for now, and with Flow v0.92, I didn't succeed in finding a way to get Flow to infer this.

gnprice added 6 commits April 11, 2019 16:23
One of many type errors that the new, actually-effective
`connect` type catches.
As mentioned in the commit message in a36814e which originally
added these fixmes:

  In fact we'd need fixmes on more of them... if the react-redux
  libdef didn't defeat type-checking at `connect` calls.

We're about to get a version of that libdef (plus our own wrapper)
that *doesn't* defeat type-checking at `connect`... so a bunch more
of these will need fixmes.  Just write one fixme to cover them all.
This just picks one of the N overloads from the libdef -- the only one
we actually need -- and specifies that particular type.  Plus fixes it
up slightly; e.g. the libdef uses the pseudotype `Object`, which
doesn't serve any purpose here that `{}` doesn't do better.

This will be helpful in a bit for isolating changes to the libdef and
to the Flow version from our code, giving us an easy single point of
control for the types Flow sees.
This converts every reference to `connect` in our codebase to
go through this wrapper, giving us an easy single point of
control for the types Flow sees for `connect`.

If in the future we start using other features of `connect`
that the wrapper ignores, we can add them there; or add a
second wrapper with a different name, if that happens to be
easier.  (No reason we have to rely as heavily on overloading
as the upstream `connect` does.)

Done mechanically, like so:

  $ perl -i -0pe '
      s<^import \{ connect \} from .react-redux..\n>
       <>m || next;
      s<from .[./]*/types.;\n\K>
       <import { connect } from '\''../react-redux'\'';\n>ms;
    ' src/**/*.js
These are the call sites that show type errors when we
switch to the upcoming improved `connect` type that lets
the type-checker work.

Done mechanically.  While on a version where `connect` had
the improved type, ran the following command:

  $ node_modules/.bin/flow status --json \
    | jq -r '
        .errors | map(.message[0].loc.source)[] ' \
    | sort -u \
    | xargs -r perl -i -0pe '
        s/\bconnect\b/connectFlowFixMe/g '

where the first few steps of the pipeline identify exactly the
files where Flow is reporting errors.  Then reviewed the diff
and fixed up by hand the one place (in RealmScreen) where the
word "connect" appeared as a false positive.
gnprice added 3 commits April 17, 2019 12:54
The libdef here is the updated one from `flow-typed`.  In that repo
it's marked as being for Flow v0.89 and up... but in reality it works
just as well with our current Flow v0.78.

OTOH the libdef has a couple of limitations:

 * It doesn't seem to understand React's `defaultProps` mechanism;
   so if the inner component has props that are made optional by
   having defaults, the outer component ends up requiring them at
   call sites.

 * It leaves some type variables in "input positions" that really
   ought to be inferrable from the arguments passed to `connect`.

   When such a type makes it into a module export, Flow wants an
   annotation for it, and a change in Flow v0.85 makes it much more
   consistent about insisting on such annotations:
     https://medium.com/flow-type/asking-for-required-annotations-64d4f9c1edf8

   With the libdef's type for `connect`, that means extra
   boilerplate to redundantly spell out the props that the outer
   component accepts, that `mapStateToProps` returns, etc.

So, we also add a wrapper that fixes these limitations:

 * For `defaultProps`, Flow has a quite effective built-in hack,
   called `ElementConfig`.  We just need to use it.

 * Fixing the boilerplate-annotations issue was trickier, but was
   accomplished by eliminating the "input" types (notably that of
   the outer component's "own props") as type variables, and instead
   carefully writing out how to infer them from other variables.

The result is that even with Flow v0.92 (the version associated with
React Native v0.59), no new annotation is required at most call sites!
(The one exception is the return type of `mapStateToProps`, and only
in cases where it takes its second, "props", argument.)

And, at the same time, we get more effective type-checking of
`connect` calls than we've ever had before:

 * Flow *does* give an error if the selector returns a wrong type.
   This is true even if the wrong type happens to be a supertype
   of what's in Props (e.g., `?Dimensions` for `Dimensions`).

 * Bonus: if the selector returns a *subtype* of what Props asks
   for -- i.e., it provides all the needed assumptions *and then
   some* -- there is no error.  That bit is easy to get wrong with
   $Diff; that's what the `$ObjMap<..., () => mixed>` is handling.

 * Also if a prop is missing entirely, Flow gives an error, just
   as in the status quo.  (The error is at call sites, because it
   just lets the prop through into the outer props.)

[Comments mainly written 2019-04-08; commit message 04-11.]
I.e., a comment placed so Flow will show it in error messages. :)
This is the one kind of annotation we do need at some call sites
of `connect`.  Specifically:
 * when `mapStateToProps` actually takes a `props` parameter,
 * we need to annotate its *return* type, as here.

Before merging the Flow upgrade, we'll need to do the same thing on
the other call sites.  Fortunately that shouldn't be much work.
@gnprice gnprice merged commit 165e848 into zulip:master Apr 17, 2019
@gnprice
Copy link
Member Author

gnprice commented Apr 17, 2019

Thanks @borisyankov for the brief discussion in audio chat the other day. Merged, after adjusting the main commit's message for the fact I mentioned above, that we do need a few annotations even with this version.

@gnprice gnprice deleted the connect-new branch April 17, 2019 20:18
@gnprice gnprice mentioned this pull request Apr 17, 2019
@gnprice
Copy link
Member Author

gnprice commented Apr 17, 2019

BTW, just to help me close a tab with confidence -- here's a Flow playground link with some of my experiments that led to the type-wrapper in f8c9810. Most of the ideas wound up in f8c9810, and with much more explanation than I wrote down in the experiments; but there are a few ideas there that didn't end up being necessary.

gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Jul 25, 2019
Fixes zulip#3399!

The bulk of the work of this upgrade went into a large number of
commits previous to this one.  The majority of the last few dozen
commits, after 1c86488^, were part of the upgrade; some discussion
in zulip#3561.  An earlier wave of effort focused on getting things working
with the upgrade of Flow to v0.92; see zulip#3450 and especially f8c9810,
and then zulip#3453 and zulip#3442.  Some other early discussion is at zulip#3422.

This commit itself was done mainly by running:

  $ tools/upgrade rn v0.59.10

Then, that tool needs a bit of an update for changes upstream.
Because I'm feeling pressed to get this upgrade out the door (and
to deal with the various separate trouble on iOS), I just took
care of the other relevant packages by hand: updated `@babel/core`
and `metro-react-native-babel-preset` in package.json according to
the diff seen in `rn-diff-purge`, then reran `yarn`.
@gnprice
Copy link
Member Author

gnprice commented Jan 25, 2020

Even this annotation seems like it shouldn't fundamentally be necessary; but at least for now, and with Flow v0.92, I didn't succeed in finding a way to get Flow to infer this.

FTR based on some quick experiments just now, I can't actually reproduce why this part appeared to be necessary!

There is #3768, which is a Flow unsoundness bug (that we just recently spotted) affecting these places in our code; but adding SelectorProps doesn't seem to do any good with that. And when I apply the silly const y = () => <X />; workaround described on that issue, everything seems to get inferred perfectly fine with no SelectorProps annotation.

@gnprice gnprice mentioned this pull request Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant