Flow-related work on the path to RN v0.64 upgrade, part 1. #4518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Normally, we wouldn't upgrade Flow except in the same commit as we upgrade React Native. That's because the code for a React Native release (the code in node_modules/react-native) is only guaranteed to pass type-checking with the Flow version that was used to check the code at that release. Given that this commit breaks from our normal practice, it's unsurprising to see a Flow error in a file in node_modules/react-native. Suppress type-checking in that specific file (it's just one file), noting that it's not a file that we use directly, nor does it seem likely that the suppression propagates to a type-checking lack in our own project. (The most relevant-looking site I've seen, our use of `UIManager` in src/ZulipMobile.js, still has its types after this change.) The nice thing we get in Flow v0.125 is that `$FlowIssue` and `$FlowExpectedError` are added as default suppression comments [1], which will allow us to remove the `suppress_comment` lines in our Flow config that define those. And that's desirable because `suppress_comment` has been removed from Flow in v0.127 [2], and we'll have to take that removal along with the Flow version we take with RN v0.64. Best, then, to handle as much of this change as we can before the main RN v0.64 upgrade commit. In our own code, we have to slightly relocate a suppression comment for facebook/flow#8276; so, do that. [1] https://github.com/facebook/flow/blob/master/Changelog.md#01250 [2] https://flow.org/en/docs/config/options/#toc-suppress-comment-regex
We don't actually use this.
And change its uses to `$FlowFixMe`. `suppress_comment` is removed in Flow v0.127.0 [1], and we'll only be able to use the built-in suppression syntax from that version on. `$FlowFixMe` isn't the ideal replacement for `$FlowMigrationFudge`; `$FlowIgnore` would match the meaning more closely. But `$FlowIgnore` doesn't get added to the built-in suppression syntax until Flow v0.128.0 (facebook/flow@ba78ab9bf). Fortunately, it seems quite straightforward to take Flow v0.128 before the main RN v0.64 upgrade commit. So, we'll do that later in this series, and switch these uses to `$FlowIgnore` afterward. [1] https://flow.org/en/docs/config/options/#toc-suppress-comment-regex
Now, we're not erasing any of the default suppression comments, and we're not adding our own. So at this point we're relying on exactly the default set of suppression comments. This is a requirement as of Flow v0.127. [1] Follows the template-app change in facebook/react-native@afad239d7, on the path to the RN v0.64 upgrade. [1] https://flow.org/en/docs/config/options/#toc-suppress-comment-regex
As noted in a recent commit where we upgraded to Flow v0.125, we don't usually bump the Flow version outside of a React Native upgrade commit. But I found that we could get v0.126 without any added Flow errors in React Native code -- which is even better than the situation with Flow v0.125, where we saw one Flow error in one file. Being on v0.126 is nice because we'll finally be able to drop our comments like "this should really be exact -- see note in jsonable.js" on many objects with indexer properties, and make those object types exact. - That, in turn, will help us address zulip#3452 more productively ("Convert all object types to exact, or explicitly-inexact"). - Fixing zulip#3452 is really a prerequisite for taking facebook/react-native@050a7dd01 (switching on `exact_by_default`), which is on the path to the RN v0.64 upgrade. [1] https://flow.org/en/docs/config/options/#toc-exact-by-default-boolean
The definition is identical [1]:
```
type $ArrayLike<T> = {
[indexer: number]: T,
length: number,
...
}
```
[1] https://github.com/facebook/flow/blob/v0.126.1/lib/core.js#L309
If all has gone to plan, I've improved *all* relevant object types in our own code this way (not including libdefs). I cast a wide net for all indexer properties, with a global search for \]: in *.js files outside the flow-typed directory. Some results weren't indexer properties, but among the ones that were, I made the inexact ones exact.
…defs. Specifically, the libdefs in flow-typed but not in flow-typed/npm. Done with the same find-and-replace strategy as in a recent commit. Here, there were substantially more *explicitly* inexact object types with indexer properties. It doesn't make a lot of sense why one would want an inexact object with an indexer property, whether implicitly or explicitly inexact. So, make those exact too.
So we can compute the latter based on the former, in the next commit.
…id`. The immediate motivation is to resolve a complaint from Flow about a situation that "may lead to an exponentially large number of cases to reason about", when we upgrade to Flow v0.128. But also, I think `sender_id` makes more sense as an alias for passing a whole unique `sender` with a specific ID, than an alias for passing `otherUser` with a changed ID field. I found nothing to contradict this in zulip/zulip-mobile@f59cd486e1, when `sender_id` was added.
Like with the other Flow upgrades in this series, we're departing from our usual practice of only upgrading Flow along with React Native. But we seem to get away with this one -- it doesn't flag any new errors in node_modules/react-native! One highlight of this version in the changelog is "Standardized error suppression syntax, added ability to suppress errors based on error codes" [1]. We couldn't have taken this upgrade without first removing our use of the `suppress_comment` option in our .flowconfig [2]. One slightly annoying feature of the standardized syntax (still present in v0.146, the latest) is that, when you put more than one error-code-specific suppression in a multi-line comment, Flow will ignore some of them. Neither the doc [3] nor the suppression-syntax tests [4] suggest that multiple suppressions in one multi-line comment is supported. We'd been using multi-line comments to keep a suppression's explanatory text (when it's too long for one line) between the suppression and the code that it's about [5]. So, to keep doing that: work around by using single-line suppression comments for all but the last error-code-specific suppression (chosen arbitrarily), and use a multi-line comment for that last one, squeezing the explanatory text in there. Apparently this Flow version finds more to complain about at some of our existing suppressions, so this workaround gets a good bit of exercise, especially in src/boot/store.js. [1] https://github.com/facebook/flow/blob/master/Changelog.md#01270 [2] https://flow.org/en/docs/config/options/#toc-suppress-comment-regex [3] https://flow.org/en/docs/errors/ [4] https://github.com/facebook/flow/blob/v0.146.0/tests/error_codes/test.js [5] zulip#4433 (comment)
It looks like we can get away with yet another of these Flow upgrades, before the main RN v0.64 upgrade commit! See a few others, earlier in this series. What we want from this version is facebook/flow@ba78ab9bf, which adds `$FlowIgnore` to the standard suppression syntax.
…ge`. As we said we'd do (in a recent commit that removed `$FlowMigrationFudge`) as soon as `$FlowIgnore` became available. Well, it's available now: the recent upgrade to Flow v0.128 brought us facebook/flow@ba78ab9bf, which adds `$FlowIgnore` to the standard/acceptable suppression syntax.
gnprice
reviewed
Mar 10, 2021
Comment on lines
-78
to
+77
| | 'onError', | ||
| >; | ||
| | 'onError', >; |
There was a problem hiding this comment.
Huh, weird. But I reproduce that that's what auto-formatting does... and this is not code we spend a lot of time in or really fully maintain, so probably better to just let it do as it likes here rather than try to persuade it to do something else. 🤷
Comment on lines
-543
to
+545
| > = { | ||
| > = {| | ||
| +[key: string]: (...args: any) => (Action | State => Action), | ||
| ... | ||
| }; | ||
| |}; |
There was a problem hiding this comment.
Here, there were substantially more *explicitly* inexact object
types with indexer properties. It doesn't make a lot of sense why
one would want an inexact object with an indexer property, whether
implicitly or explicitly inexact. So, make those exact too.
Hmm, yeah.
These are mostly lightly-edited flowgen output, right? Seems like probably a (minor) bug in flowgen.
|
Thanks for the quick review! |
This was referenced Mar 10, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves the long-standing comments dotting our code like "this really should be an exact object type -- see note in jsonable.js about objects with indexer properties."
As one might remember from d1affaf, that was something we weren't expecting to be able to do until Flow v0.126. We're on Flow v0.122, and the latest release candidate for RN v0.64 is on Flow v0.137. So we generally wouldn't expect this work to be doable until after we're on RN v0.64; that's #4426.
Well, my solution is to actually take the Flow v0.126 upgrade now, even before upgrading React Native. I also take a couple more Flow upgrades, through v0.128, for some other things.
Our normal practice is to change the Flow version only in the same commit that we change the React Native version. That's because the code for a React Native release (the code in node_modules/react-native) is only guaranteed to pass type-checking with the Flow version that was used to check the code at that release. Using a different version of Flow means we might run into errors in node_modules/react-native, and we depend on that being error-free, and without suppressions, because we want our use of React Native's APIs to be type-checked.
But I think the benefits outweigh the costs in this case:
exact_by_default), and that's on the path to the RN v0.64 upgrade. That commit message warns that React Native's code may assume that projects turn onexact_by_default.$FlowMigrationFudgeand our other uses ofsuppress_comment(as Greg noted we'd have to) now instead of later. Including the ability, with Flow v0.128, to use$FlowIgnoreinstead of$FlowFixMein the places we used to use$FlowMigrationFudge.node_modules/react-native/Libraries/ReactNative/PaperUIManager.js. I've never heard of this file, we never import it, and I don't expect we'll cause any problems by temporarily (until we're on RN v0.64) telling Flow to ignore it.I have a draft on top of this that goes further toward #3452. The "implicit-inexact-object" lint Greg mentions at #3452 (comment) finds 192 warnings (142 in our own code, the rest in libdefs) at the current
master(before this PR). At the tip of that draft in its current state, it finds just 54 warnings, with 19 in our own code, and those numbers are decreasing 😄.