zulip/zulip-mobile

Cover our code with Flow types

Open

#2,052 opened on Mar 13, 2018

View on GitHub
 (21 comments) (0 reactions) (0 assignees)JavaScript (673 forks)github user discovery
help wanted

Repository metrics

Stars
 (1,348 stars)
PR merge metrics
 (PR metrics pending)

Description

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!

Contributor guide