Cover our code with Flow types

[gnprice]

2022 update: There's one area of this issue where help is still appreciated: adding types to the remaining 16 files, all in our test suite, that don't yet have them.

Here's a to-do list (as of 1e89120a2e8a7893fb797fba56fd2c1ae0133eea), sorted by size:

$ git grep -L @flow src/ | grep js$ | xargs wc -l | sort -rn
 2261 total
  368 src/streams/__tests__/streamsReducer-test.js
  359 src/chat/__tests__/flagsReducer-test.js
  303 src/user-groups/__tests__/userGroupsReducer-test.js
  233 src/presence/__tests__/presenceReducer-test.js
  184 src/topics/__tests__/topicsReducer-test.js
  165 src/emoji/__tests__/emojiSelectors-test.js
  103 src/utils/__tests__/unread-test.js
  102 src/reactions/__tests__/aggregateReactions-test.js
   83 src/utils/__tests__/immutability-test.js
   82 src/webview/html/__tests__/processAlertWords-test.js
   70 src/utils/__tests__/date-test.js
   64 src/alertWords/__tests__/alertWordsReducer-test.js
   45 src/api/__tests__/apiFetch-test.js
   42 src/account/__tests__/accountsSelectors-test.js
   30 src/chat/__tests__/fetchingSelectors-test.js
   28 src/caughtup/__tests__/caughtUpSelectors-test.js

For the current status of the few other remaining areas of type-uncovered code, see: https://github.com/zulip/zulip-mobile/issues/2052#issuecomment-1088126025

---

Original description follows:

Static types can make a big difference in making it easier to understand a codebase. That's valuable because it helps us write more reliable code with fewer bugs, and making the app more reliable with fewer bugs is our top priority right now. It's also valuable because it helps people learn the codebase more quickly, and we'll likely have several people learning the codebase over the next few months for GSoC.

We use Flow for static types in the Zulip mobile app, but there's still a lot of code that doesn't have types. Let's add more. Here's one way to do it:

1. Look in src/types.js for a type definition that just says any and then has a real, more detailed type commented out.
2. Replace any with a real type. Try the one commented out next to it. This type is probably wrong but is a good guess to start from.
3. Run yarn run test:flow and see what errors you get. Study the errors and the surrounding code to figure out what the type should really be. Read the Flow docs to help understand what the types mean.
4. Try fixing the types. This might mean editing the type definition you just added; or editing the code that uses it to refer to some other type instead; or editing other type definitions; or some other fix. Use what you've learned from studying the code!
5. Repeat steps 3 and 4 until you get no errors.
6. Commit your work as clear, coherent commits. Use the commit messages to explain anything you learned that won't be obvious to someone reading the commits. Then send a PR :smile:

Some other ways to find untyped code:

• You can run yarn run test:flow-coverage to get a nice list of files with lots of uncovered code, and click on the filenames to see specific lines. (Also helpful is yarn run test:flow coverage --color to see details on any one file.)
• You can run yarn run test:flow-todo to see a list of places we could have type annotations but don't, or have one like any that doesn't mean anything.

You can do either of those in place of step 1 above -- then steps 2 to 6 are similar.

Here's a few examples of what a good commit for this can look like:

• 835e117ab a simple example
• 212da36d7 where the right thing was to remove that type and fix the annotations
• c1c68cb75 a more complex example
• ac04b1099a another complex example (though splitting into several commits would have made it even better)

For tracking our progress: right now there are

• 44 uses of any or Object in src/types.js
• 324 total uses of any or Object found by yarn run test:flow-todo
• 5748 expressions of unknown type in the codebase, 20.2% of the 28462 total expressions (as counted by yarn run test:flow-coverage).

Let's drive those all toward zero!

---

Update 2018-09-27 (commit 0cbb212bf):

• 23 = 8 + 10 + 5 uses of any or Object in src/types.js, src/actionTypes.js, and src/api/apiTypes.js (we've split the file into pieces since the original figures above)
• 219 total uses of any or Object found by yarn run test:flow-todo
• 2055 expressions of unknown type in the codebase, 7.1% of the 29034 total expressions (as counted by yarn run test:flow-coverage)

That's about 65% less uncovered code than we had in March! We've eliminated somewhat smaller fractions of the other figures, which is appropriate -- it means we've been focusing on the anys with the biggest impact on the code.

Still more to do. :smile: #2971 is a great source of high-value places to add types. Running yarn run test:flow-coverage remains another great source.

---

Update 2019-12-31 (commit 9f2e1be90):

• 0 :tada: uses of any or Object in src/types.js, src/actionTypes.js, src/api/apiTypes.js, or any similar file
• 11 total uses of any or Object found by yarn run test:flow-todo
• 1181 expressions of unknown type in the codebase, 4.0% of the 29624 total expressions (as counted by yarn run test:flow-coverage).

That's very nearly zero uses of any or Object! :confetti_ball: It's also about 43% less uncovered code than we had late last year. The rate of decrease there has slowed as we've taken care of a lot of the larger swathes of often-edited code.

Still more to do. Running yarn run test:flow-coverage is the best way to get a picture of good places to add types. In the report that that produces, try sorting by "Uncovered", and then clicking through to the reports on individual files that have a lot of uncovered expressions.

---

Update 2021-10-06 (commit a4ada6581):

• 0 :tada: total uses of any or Object found by yarn run test:flow-todo
• 2108 expressions of unknown type in the codebase, 4.6% of the 45550 total expressions (as counted by yarn run test:flow-coverage)

So we've completely eliminated the use of any or Object in our code! In general we expect to keep it that way, because we systematically use @flow strict-local in each file, and we have Flow configured to enforce the unclear-type flowlint in strict mode. (We should probably add a quick lint to ensure we keep using strict-local everywhere, but it's something we rarely miss; we celebrated strict-local everywhere in 397d427ca, and in the 2.5 years since then it looks like just one file without it has snuck back in.)

That leaves the expressions of unknown type, as found by yarn run test:flow-coverage. We've regressed on that in the couple of years since the last update. The bulk of that regression is from the 719 expressions in src/third/redux-persist/, which is a library we vendored, completely untyped, and haven't yet gone and added types to. (-> #5031)

The biggest other opportunities for adding coverage are in messageActionSheet.js, eventToAction.js, js.js, and replaceRevive.js. For details on those, run yarn run test:flow-coverage and take a look!

---

Update 2022-04-04 (commit 1e89120a2):

• 1069 expressions of unknown type, 2.2% of the 49189 total expressions (as counted by yarn run test:flow-coverage)

The major improvements since the last update are from #5031 adding types to redux-persist, and from #5219 upgrading to Flow v0.141 as described at https://github.com/zulip/zulip-mobile/issues/2052#issuecomment-934685281 . Our type coverage is now the best it's ever been.

The biggest remaining opportunities for adding coverage are in eventToAction.js, replaceRevive.js, js.js, and ChatNavBar.js. For details on those, run yarn run test:flow-coverage and take a look!

[harishnandan]

@gnprice I would like to work on this.

[gnprice]

Great -- please do!

This is a project with lots and lots of independent pieces to do, so it's perfect for several people to work on at once. (In fact if you look in the commit history, you'll see that both Boris and I have already been doing some of this work.) So this is very much still open for more people to work on too -- the more the merrier.

[gokcan]

Hello @gnprice!

I have both personal and professional work experience with types, flow and also TypeScript. So I would like to work on this issue as a GSoC applicant if there is a need.

[borisyankov]

@gokcan feel free to start contributing on this before the GSoC (as a means to help us evaluate you and yourself during the proposal phase).

Doing small PRs covering a single file or area is a good way to make sure the code gets merged faster and that work doesn't overlap with other contributors.

[gokcan]

@borisyankov Okay, got it.

[mayank4kathuria]

@zulipbot claim

[zulipbot]

Welcome to Zulip, @mayank4kathuria! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip-mobile/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

• Join me on the Zulip developers' server, to get help, chat about this issue, and meet the other developers.
• Sign the Dropbox Contributor License Agreement, so that Zulip can use your code.
• Unwatch this repository, so that you don't get 100 emails a day.

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

[gokcan]

@zulipbot claim

[zulipbot]

Hello @gokcan, it looks like someone has already claimed this issue! Since we believe multiple assignments to the same issue may cause some confusion, we encourage you to search for other unclaimed issues to work on. However, you can always reclaim this issue if no one is working on it.

We look forward to your valuable contributions!

[borisyankov]

@gokcan feel free to work on part of this without claiming this issue as it can be broken down into many small tasks and multiple contributors can work on simultaneously.

[ghost]

Hello, I would like to contribute to your organisation starting here. Can I work on this issue?

[borisyankov]

@flowkraD sure. This is a good area to work on as a first contribution.

[mbuotidem]

Hi @borisyankov @gnprice We're doing a class where we need to work on Open Source software and help them fix issues and my group thought this issue was approachable enough. We're trying to get setup and following the Windows guide but its a bit confusing for complete beginners. We'd be happy to work with you to figure it out and then do a PR to update the documentation. Is there an IRC where we can talk with someone? We don't want to litter this issue with troubleshooting questions.

[ghost]

The official chat is chat.zulip.org as mentioned here. I wouldn't recommend pinging the devs cause you might wake them up in middle of night if they are on the other side of the world.

[borisyankov]

As @flowkraD says you are welcome to share your thoughts or ask any questions in #mobile

[vk-mittal14]

I am new to open source and zulip mobile. Can I start working on this issue?

[gnprice]

@i-m-vivek Certainly! Please do.

This is a project with lots and lots of independent pieces to do, so it's perfect for several people to work on at once.

[parshva-b]

Hey, I am Parshva Barbhaya, and am new to open source as well. After reviewing the thread I realized there are multiple people who can work on this issue, so can I also contribute? I want to participate in GSoC 2020.

My areas of interest are JavaScript, React

[gnprice]

I've updated the description with where things stand now.

Note that in some of the files that have only a small number of uncovered expressions, what's happening is in chains of methods like Array#map and Array#filter, where the elements of the array are being inferred for some reason as types like any | number (for example) where they should be number. This seems to be harmless -- if you try to use the elements in a way incompatible with the actual type, you get an error -- but shows up in the coverage report. In any case, those are fixed in Flow v0.141: https://github.com/facebook/flow/blob/main/Changelog.md#01410

Improved inference of chained generic method calls, such as Array methods. For example, given [1, 2].map(a => a).forEach(b => b), Flow now infers that b is a number rather than any | number.

I just tried upgrading to that version, and there are a handful of errors to fix, but it made quite an improvement in coverage: about 480 fewer untyped expressions, including nearly all of what was uncovered in messageActionSheet.js. So no need to do anything in our code to work around that issue and cover those sorts of expressions.

[Anshul-9923]

@gnprice I would like to work on this issue. Can you please assign it to me?

[gnprice]

I've updated the description: we're now down to about 98% coverage, with about 1000 uncovered expressions remaining, according to yarn run test:flow-coverage.

I've also gone and done a sweep looking through what's left. #5323 will fix a good chunk of those, leaving a bit under 1000 expressions still uncovered. The remaining areas are:

• Adding types on the remaining few test files that lack them (which are the only files that lack them.) These actually aren't counted in those figures from yarn run test:flow-coverage -- they probably add up to more than all the rest.
• Our initial processing of server events in eventToAction.js. As it happens, I have a draft branch I started the other day in the direction of fixing this.
• replaceRevive.js, which I think is unlikely to be actionable.
• js.js (and other files in src/webview/js/), where we use the global window. This is facebook/flow#6709.
• Writing down types for our various native modules. Search for NativeModules to find these (plus a couple of places where we use an external library that way.) These should get either libdefs, or probably better small JS-side wrappers that provide types.
• Adding a libdef for lodash, or rather the few bits of it we use.

• Adding libdefs for a few other libraries: color, @expo/react-native-action-sheet, redux-thunk, and others.

  For a complete list: $ git grep -h -A1 '\$FlowFixMe\[untyped-import\]' | grep -Eo 'from \S+;$' | sort | uniq -c | sort -n

• The type of RN's Platform.select is too complicated, in a way that can defeat type inference (in particular, in src/common/Input.js). We could work around this with some verbosity, but I'll just send a PR upstream (edit: https://github.com/facebook/react-native/pull/33567) to fix it there. (There's no huge urgency to get better type coverage on the handful of places we use Platform.select, so no need to backport the fix to a fork or anything in the meantime.)
• A scattering of cases of facebook/flow#2470. Even with the workaround we use (search our code for 2470 to find examples), there's two expressions reported as uncovered at each place we catch an exception. It's not possible to remove those short of that Flow issue getting resolved.
• At each call to ensureUnreachable, the argument must have type empty and is therefore reported as uncovered. That type being empty means it's working as designed -- the point is to assert to the type-checker that it should be able to verify that control-flow path is unreachable -- so these are unactionable.

---

When I first filed this issue four years ago, there were a number of places where we had types annotated as any for things we passed around within our own code. Fixing those made a good first issue for newcomers, because when trying to write down a real type (replacing any) for any of those, you'd get fast and reliable feedback from Flow as it checked your type for consistency with how we used it.

Today, though, our internal anys are gone. Nearly all of the remaining areas described above are to write down types for something external: some native code, a third-party library, or a server event deserialized from JSON. This means that Flow has no way to check what assumptions we can really make about those values. So writing down the types requires a greater degree of familiarity with Flow, and also a closer investigation of those external systems, in order to get the types right in the absence of that feedback. So none of those make a "good first issue".

The exception, where the types are entirely internal and so Flow is still able to give reliable feedback to guide you, is adding types to the tests -- see our test style guide for more. This is a valuable thing to do, and I think quite accessible for a relatively new contributor, but I think not good as a first issue -- it's best done after having another PR or two merged that include some tests in our test suite.

I'm therefore leaving this marked as "help wanted" -- specifically for adding types to our tests -- but no longer as "good first issue".