Skip to content
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

Flow: Switch to new "Types-First" mode. #4907

Closed
chrisbobbe opened this issue Jul 19, 2021 · 2 comments · Fixed by #4987
Closed

Flow: Switch to new "Types-First" mode. #4907

chrisbobbe opened this issue Jul 19, 2021 · 2 comments · Fixed by #4987
Labels
a-tools upstream: RN Issues related to an issue in React Native

Comments

@chrisbobbe
Copy link
Contributor

We generally keep our Flow version at the same version as the React Native project uses at the time of the RN release we're using. That's because different Flow versions tend to flag different errors and warnings; this happens naturally as Flow fixes bugs and adds/improves features.

  • This means we should aim for Flow v0.137 in the upcoming React Native v0.64 upgrade (link); that's Upgrade to RN v0.64 #4426.
  • RN v0.65 hasn't been released yet (and we don't have an issue for it yet). But from a release candidate, it looks like we should expect to aim for v0.149 or later (link).

From the doc, it looks like Types-First was released as experimental in v0.102 and released officially in v0.125. It'll be enabled by default starting in v0.134, and that's why we'll have to either switch to it before the RN v0.64 upgrade, or temporarily opt out with types_first=false.

From the Flow changelog, I see that v0.143 removed the ability to opt out:

Support for Classic mode has been dropped and Types-First mode is now always enabled (Types-First has been the default mode since v0.134). The types_first and well_formed_exports flowconfig options are no longer recognized. See https://medium.com/flow-type/types-first-a-scalable-new-architecture-for-flow-3d8c7ba1d4eb/ for more about Types-First mode.

So, to keep up with React Native, we'll want to have Types-First enabled by the time we upgrade to RN v0.65, at the latest, since we'll want to aim for Flow v0.149 for that upgrade.

As I mentioned at #4426 (comment):

The thing we'll have to do to enable it is annotate almost all the exports of our modules:

The caveat of this new version is that it requires exported parts of the code to be annotated with types, or to be expressions whose type can be trivially inferred (for example numbers and strings).

Not annotating an export sufficiently will raise a signature-verification-failure error.

@chrisbobbe chrisbobbe added upstream: RN Issues related to an issue in React Native a-tools labels Jul 19, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 19, 2021
These were found by temporarily setting `well_formed_exports=true`
in the Flow config, as recommended in the Flow doc on Types-First:
  https://flow.org/en/docs/lang/types-first/.

This will move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 19, 2021
…easy.

These were found by temporarily setting `well_formed_exports=true`
in the Flow config, as recommended in the Flow doc on Types-First:
  https://flow.org/en/docs/lang/types-first/.

This will move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 19, 2021
… easy.

These were found by temporarily setting `well_formed_exports=true`
in the Flow config, as recommended in the Flow doc on Types-First:
  https://flow.org/en/docs/lang/types-first/.

This will move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 20, 2021
These were found by temporarily setting `well_formed_exports=true`
in the Flow config, as recommended in the Flow doc on Types-First:
  https://flow.org/en/docs/lang/types-first/.

This will move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 20, 2021
… easy.

These were found by temporarily setting `well_formed_exports=true`
in the Flow config, as recommended in the Flow doc on Types-First:
  https://flow.org/en/docs/lang/types-first/.

This will move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 21, 2021
This is much easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jul 21, 2021

OK, I've just made another pass through the errors that appear if I temporarily set well_formed_exports=true. A few exports stood out to me as particularly challenging to annotate, and I'll probably start discussions for them on CZO once we take care of the easier things:

  • action export in exampleData.js (we have a comment saying we've explicitly chosen not to annotate this)
  • default export of src/styles/index.js
  • functions where the return value is the result of a fetch (would be good to get facebook/react-native@6651b7c59 so fetch won't be untyped anymore, but that was released in v0.65, which is our deadline for turning on Types-First)
  • Thunk action creators

Other than those, I think the main challenge will be some unusually tricky conversions to function components; I see about 9 of those in the error output. (From the tip of my current revision for #4914, I see about 30 in the error output that I've marked as "easy".) But of course there's an escape hatch with those 9: we can always just work out the required annotations and add them, sacrificing some cleanliness because those might end up being pretty verbose.

@gnprice
Copy link
Member

gnprice commented Jul 24, 2021

Thanks! I took a look at this earlier today, and started a chat thread: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237897

Here's how I think we can handle those four classes of exports:

  • eg.action: I think we can do this by just exporting the individual action types like RealmInitAction, and casting each action to the appropriate one of those.
  • src/styles/index.js: looks like we can do this by writing Object.freeze for the export, and doing the type-checking that createStyleSheet does separately
  • fetch return values: The status quo here is actually already that these functions are inferred to return empty (or Promise<empty>). So we might as well write that explicitly in annotations -- I didn't realize that was happening myself until I ran into it when developing Clean up API error-distinguishing logic #4896 last week.
    • Also we should fix it, but that's independent of types-first.
  • Thunk action creators: looks like these are easy, fortunately

I'd also encourage you to try using the codemod. Its output requires a bit of fixing up, but then it completely successfully takes care of a large swath of the errors.

The major remaining class of errors looks to be connect calls. I'm happy to see those handled by converting to Hooks, as you're doing, so long as that's easy. For any components where that's hard, let's handle getting an annotation the direct way: I think it's basically a matter of writing down an OuterProps type, and then annotating the export as ComponentType<OuterProps>.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 26, 2021
This is much easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 27, 2021
This is much easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

Prefer the `Node` type exported from React over `React$Node`.
They're equivalent, but Greg prefers `Node` as it seems cleaner; see
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 28, 2021
This is much easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

Prefer the `Node` type exported from React over `React$Node`.
They're equivalent, but Greg prefers `Node` as it seems cleaner; see
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 28, 2021
This is much easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

Prefer the `Node` type exported from React over `React$Node`.
They're equivalent, but Greg prefers `Node` as it seems cleaner; see
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 28, 2021
This is easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

Prefer the `Node` type exported from React over `React$Node`.
They're equivalent, but Greg prefers `Node` as it seems cleaner; see
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237907.

Done by using the codemod [1] with the following command, then
removing irrelevant changes, then making the annotations a bit more
concise.

  yarn flow codemod annotate-exports --write --repeat \
    --log-level info src/ 2>out3.log

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

[1] https://flow.org/en/docs/cli/annotate-exports/
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237928.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
…track.

Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237936.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
The `empty` is bogus, as we saw in 4ef0f06. As mentioned there,
it's because React Native defeats type-checking on `fetch` calls. We
expect that'll be fixed in RN v0.65, with
facebook/react-native@6651b7c59.

But in the meantime, if we're going to switch to Flow's types-first
mode (zulip#4907), then we need to annotate all our exports. As noted in
the issue, we have a deadline of RN v0.65 to get that done.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 4, 2021
Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237907.

Done by using the codemod [1] with the following command, then
removing irrelevant changes, then making the annotations a bit more
concise.

  yarn flow codemod annotate-exports --write --repeat \
    --log-level info src/ 2>out3.log

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

[1] https://flow.org/en/docs/cli/annotate-exports/
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 4, 2021
Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237928.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 4, 2021
…track.

Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237936.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 4, 2021
To bring us closer to annotating all our exports, for zulip#4907.

Most of these use a new `FixmeUntypedFetchResult` type; see its
jsdoc.

`apiFetch` can be unexported; so, do that, removing the need for an
annotation [1].

`savePushToken` and `forgetPushToken` get `Promise<mixed>` because
we don't actually inspect the returned result at all [2].

`checkCompatibility` gets `Promise<{ status: number, ... }>` because
we'll always be looking at the response status [3].

[1] zulip#4927 (comment)
[2] zulip#4927 (comment)
[3] zulip#4927 (comment)
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 4, 2021
Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237928.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

We move the `deepFreeze` inward, onto each individual member of
`eg.action`, because it isn't transparent to the types-first quick
pass; see
  zulip#4927 (comment).
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 4, 2021
…track.

Following Greg's recommendation at
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1237936.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

And chase through many Flow errors pointing out that various other
object and array types should be marked read-only too. All of them
look like they should be read-only anyway; our read-only markers
haven't generally kept up with all the places we pass around objects
and arrays expecting them not to be mutated.

After adding the annotation, I went through five iterations of

- run `yarn flow`
- resolve each error in the output (and no others) by
  command-clicking to the type that Flow says should be made
  read-only, and making it so

, stopping when Flow didn't show any more errors.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This way, we won't have to annotate `defaultProps` for zulip#4907 in
either of the following ways:

- by using `$Shape`, which is unsound,
- by spelling out the properties that happen to appear there,
- or by doing something more complicated, like with `$ObjMap`

This has been the reason for converting several other components on
the way to zulip#4907, but we haven't written it down until now.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

And, since one consumer doesn't like the annotation, just repeat
NULL_OBJECT's expression there, this time with a note about why
we're using `Object.freeze`.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 12, 2021
This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

And chase through many Flow errors pointing out that various other
object and array types should be marked read-only too. All of them
look like they should be read-only anyway; our read-only markers
haven't generally kept up with all the places we pass around objects
and arrays expecting them not to be mutated.

After adding the annotation, I went through five iterations of

- run `yarn flow`
- resolve each error in the output (and no others) by
  command-clicking to the type that Flow says should be made
  read-only, and making it so

, stopping when Flow didn't show any more errors.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
This is easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

Prefer the `Node` type exported from React over `React$Node`.
They're equivalent, but Greg prefers `Node` as it seems cleaner; see
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
This is easier to do now that we've converted these to function
components in this series.

This will help move us along toward Flow's new "Types-First" mode;
switching entirely is zulip#4907.

Prefer the `Node` type exported from React over `React$Node`.
They're equivalent, but Greg prefers `Node` as it seems cleaner; see
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
We're not totally satisfied with how we annotate `defaultProps` (see
f801c97), but it works, and we'd like to remove `Centerer` anyway;
issue zulip#4868 is in progress toward that.

Related: zulip#4907
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
We're not totally satisfied with how we annotate `defaultProps` (see
f801c97), but it works, and we'd like to remove `Centerer` anyway;
issue zulip#4868 is in progress toward that.

Related: zulip#4907
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 3, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 3, 2021
From Flow's perspective, we're fine to delay types-first until we're
on v0.143, which we'd normally take along with the RN v0.65 upgrade.

But, from experimentation, React Native v0.64's internal code seems
to have been written with Types-First, such that a few Flow errors
would show up if we tried to use RN v0.64 without Types-First
enabled. So, enable it, now that we can, and now that the RN v0.64
upgrade is on the horizon.

Fixes: zulip#4907
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 3, 2021
From Flow's perspective, we're fine to delay types-first until we're
on v0.143, which we'd normally take along with the RN v0.65 upgrade.

But, from experimentation, React Native v0.64's internal code seems
to have been written with Types-First, such that a few Flow errors
would show up if we tried to use RN v0.64 without Types-First
enabled. So, enable it, now that we can, and now that the RN v0.64
upgrade is on the horizon (zulip#4426).

Fixes: zulip#4907
@gnprice gnprice closed this as completed in 4774b91 Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-tools upstream: RN Issues related to an issue in React Native
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants