Begin adding Flow annotations #2196

kumar303 · 2017-03-27T20:33:04Z

I have entered into the valley of darkness [1] to seek out the Light of Truth. Who will walk with me?

I started by annotating one component (I swear). I tried to follow its dependencies and annotate each one but I didn't finish because we have so much shared logic. I think this paints an interesting picture of what Flow can buy us.

Here are some observations:

Declaring API response objects will be very helpful not only in catching errors but also in understanding how to work with the API. In my own experience, one already has to start by understanding the API data (either from the docs or with browser devtools) so using Flow will simply require us to document our findings.
Declaring state objects will be very helpful for all kinds of things (reducers, components) because we have lots of code that depends on specific state structures. It has been pretty hard to catch all mistakes using our test suite.
We can finally validate our use of component properties!
Static typing reveals the truth about what your code is trying to accomplish and I have already spotted a few code problems after adding annotations.
Flow adds verbosity and the syntax sometimes looks odd but I think the verbosity will make understanding how objects interact with each other much easier.
Flow-typing tests is going to be hard because it will require some architectural changes around dependency injection. It may take longer to use Flow in our tests.

Let me know what you think!

[1] during my 8-hour flight back from London to Chicago

kumar303 · 2017-04-10T22:54:02Z

@tofumatt thanks for the thorough review. I expanded the docs to help clarify everything and addressed each issue. Can you take another look?

tofumatt

I'm curious about those single pipes in the type declarations and the filenames... mostly the rest of the stuff is r+wc types of things. This is looking great, I'm definitely getting more comfortable with the syntax and the comments/docs looks great.

tofumatt · 2017-04-12T12:26:11Z

README.md

+
+Here is our Flow manifesto:
+
+* We use Flow to **declare the intention of our code** and help others


These docs are really awesome now, thanks so much for doing another pass. Super easy to grok and the links are great 👍

tofumatt · 2017-04-12T12:27:33Z

README.md

+  'Inexact type is incompatible with exact type' but that's a
+  [bug](https://github.com/facebook/flow/issues/2405).
+  You can use the `Exact<T>` workaround from
+  [`src/core/types/coreTypes`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/coreTypes.js)


Why do we call the file coreTypes when it's already in the types/ dir? I think I at least meant to ask but forgot or it got buried. Seems redundant though.

Oh, I did change the name but I forgot to update this link! Fixed.

tofumatt · 2017-04-12T12:27:54Z

README.md

+  into a bug or if you hit something that's making you bang your head on
+  the keyboard. If it's something you think is unfixable then use
+  `$FLOW_IGNORE` instead. Please explain your rationale in the comment and link
+  to a github issue if possible.


Nitpick but GitHub is a proper noun.

tofumatt · 2017-04-12T12:29:04Z

package.json

    "dev:disco": "better-npm-run dev:disco",
    "eslint": "eslint .",
+    "flow:check": "flow check",
+    "flow:dev": "chokidar .flowconfig flow/ src/ tests/ -i flow/logs/flow.log -c 'flow status' --initial",


I forget, is this safe to run if you don't have chokidar installed globally? I'm rusty on my npm run command knowledge. Maybe it's okay?

Yep, it's safe. "In addition to the shell's pre-existing PATH, npm run adds node_modules/.bin to the PATH provided to scripts."

tofumatt · 2017-04-12T12:31:50Z

src/amo/api/index.js

  return new Promise(
    (resolve) => {
-      const data = { rating, version: versionId, body, title };
+      const data = {


Nitpick but postData, ratingData, or reviewData might be a bit less generic. data is a vague variable name.

Sure, I changed it. However, I feel like a short and concise variable name makes code easier to understand when it only spans 10 lines or so. Otherwise the verbosity can slow a reader down.

Actually, naming it review instead achieves the same conciseness that data provided.

Sweet, I didn't even consider that, oops 😲

tofumatt · 2017-04-12T12:36:41Z

src/core/api/index.js

 }

+type CallApiParams = {|
+  endpoint: string,


This would be nice to alphabetise I think.

I did another sweep and I think all param lists should be alphabetized now

tofumatt · 2017-04-12T12:44:03Z

src/core/types/addonTypes.js

+|};
+
+export type AddonTypeProp =
+  typeof ADDON_TYPE_DICT |


These look like they should be || ORs not bitwise ORs, as I assume we want string here not 0. Odd that the type being 0 didn't mess stuff up somewhere...

tofumatt · 2017-04-12T12:46:35Z

src/core/types/addonTypes.js

+  review_url: string,
+  slug: string,
+  status: 'beta'
+    | 'lite'


Oh, I see single pipes being used here too. I know the pipes can surround the object syntax and it's something special in Flow. But are they also used as regular OR in Flow? What would a double pipe here do? Normally I'm used to catching bitwise OR operators as typos so they set off alarm bells in my head. 😖

tofumatt · 2017-04-12T12:47:09Z

src/core/types/coreTypes.js

@@ -0,0 +1,6 @@
+/* @flow */


What's the reason for wanting types in the folder and filename?

tofumatt · 2017-04-12T12:47:37Z

src/core/types/reduxTypes.js

+// It may be possible to use an official library for this.
+// See: https://github.com/reactjs/react-redux/pull/389
+// and: https://github.com/reactjs/redux/pull/1887/files#diff-46d86d39c8da613247f843ee8ca43ebc
+export type DispatchFn = (action: Object) => void;


This should be changed to DispatchFunc like other function type names.

tofumatt

Just added my comment about bitwise pipes.

GitHub was having me comment on old code so my thoughts on filenames are no longer a thing.

A few things needing cleaning up but I'm mostly wondering about the single pipes in src/core/types/addons.js.

tofumatt · 2017-04-12T14:58:20Z

src/core/types/addons.js

+  review_url: string,
+  slug: string,
+  status: 'beta'
+    | 'lite'


These look like they should be || ORs not bitwise ORs. Odd that this resulting in 0 didn't mess stuff up somewhere...

Are they also used as regular OR in Flow? What would a double pipe here do? Normally I'm used to catching bitwise OR operators as typos so they set off alarm bells in my head. 😖

It is a Union type so I think it's correct but, huh, maybe there is a bug here because it's not protecting me from referencing an invalid status value (which is what I intended). Well, at the very least it's good documentation and when the bug is fixed it will produce errors.

kumar303 · 2017-04-12T19:07:15Z

@tofumatt thanks, this helped me dig into a few more issues. It should be ready for another look.

tofumatt

Thanks for dealing with my constant nitpicks and for doing all this work. Let's hope Flow helps us bash our heads on keyboards less! 👍 😆 🎉

kumar303 added 30 commits March 25, 2017 09:47

initial Flow setup

c3c0023

ignore flow log

36b18a8

more fixups

e3869f3

moved logs

a6d999f

fixed dedent

bcabe90

api fixes, dedent fix

776f86c

removed userId, fixed action callbacks

a011fcf

better function defs

b6664e1

api actions, reducers, review actions

8ea3b11

added flow strip types

560cc22

make a separate ErrorHandler type

6d666b0

fixing tests

4db137d

dedent comments

502543e

fixed location tests

1908d7f

fixed up more tests

83c7e87

fixed setReview problems

89cd739

added missing api state types

ce4a39d

Add minimal typing to i18n/utils

b935fd7

fix i18n optional keywords

bd242ea

type hint for the i18n config

21e9c5b

fixed lint errors

3f4d74e

added missing ApiStateType

eee97ff

fixed another missing ApiStateType

92be514

added todo comment

86be2fb

complete Addon type

09e2570

fixed missing review type

cfb67b0

fixed missing ApiStateType

a5c3cdb

Merge branch 'master' into flow-iss2044

00c9e4f

updated yarn.lock

213910f

Merge branch 'master' into flow-iss2044

5078b4a

kumar303 added 7 commits April 10, 2017 11:40

made 'rabbit hole' more explicit

7085a4d

flow docs

d643a3c

reformat imports

2bc4a66

Use func not fn abbreviation

6494585

use 'any' not star

830d23d

Fn -> Func

ff60473

remove irrelevant typeof declarations

8ce880f

kumar303 mentioned this pull request May 5, 2024

Rely on Flow types for action/reducer types mozilla/addons#10331

Closed

kumar303 added 4 commits April 10, 2017 17:41

More thorough Exact<T> explanation

8d7f1d8

Merge branch 'master' into flow-iss2044

a7524a2

alphabetize

04f6e84

removed type suffix from type files

7a78042

kumar303 requested a review from tofumatt April 10, 2017 22:54

Merge branch 'master' into flow-iss2044

1a5e2e0

tofumatt suggested changes Apr 12, 2017

View reviewed changes

tofumatt reviewed Apr 12, 2017

View reviewed changes

kumar303 added 6 commits April 12, 2017 10:13

rename -> core/types/util

ee16518

capitalize GitHub

7dd4d3e

rename data -> review

2c3d08c

alphabetize all type params

f379431

add leading pipe for pretty formatting

3f512df

Merge branch 'master' into flow-iss2044

c21ed00

kumar303 requested a review from tofumatt April 12, 2017 19:06

tofumatt approved these changes Apr 13, 2017

View reviewed changes

Merge branch 'master' into flow-iss2044

52d88a0

kumar303 merged commit 3f41057 into mozilla:master Apr 17, 2017

kumar303 removed the state:pull_request_ready label Apr 17, 2017

kumar303 deleted the flow-iss2044 branch April 17, 2017 18:37


		Here is our Flow manifesto:

		* We use Flow to declare the intention of our code and help others

Begin adding Flow annotations #2196

Begin adding Flow annotations #2196

Uh oh!

Conversation

kumar303 commented Mar 27, 2017

Uh oh!

kumar303 commented Apr 10, 2017

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumar303 Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumar303 commented Apr 12, 2017

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kumar303 Apr 12, 2017 •

edited

Loading