-
Notifications
You must be signed in to change notification settings - Fork 400
Begin adding Flow annotations #2196
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
Changes from all commits
c3c0023
36b18a8
e3869f3
a6d999f
bcabe90
776f86c
a011fcf
b6664e1
8ea3b11
560cc22
6d666b0
4db137d
502543e
1908d7f
83c7e87
89cd739
ce4a39d
b935fd7
bd242ea
21e9c5b
3f4d74e
eee97ff
92be514
86be2fb
09e2570
cfb67b0
a5c3cdb
00c9e4f
213910f
5078b4a
b6c2d6c
d9d8775
248356b
3cb62c0
1411198
77ce29c
e040275
cf88101
b698e5f
71980b0
1df442b
6f23b6e
17ee0dd
56a4387
4d5b6fc
a4394a4
052e097
0ddff93
eae1a6b
f7ec38a
732cebd
c5d9b19
cd8712b
ec24254
892fe90
cf66983
48cf36f
898a45e
aa46b39
9ab7b38
68a4d8a
cec7c56
69ff789
7085a4d
d643a3c
2bc4a66
6494585
830d23d
ff60473
8ce880f
8d7f1d8
a7524a2
04f6e84
7a78042
1a5e2e0
ee16518
7dd4d3e
2c3d08c
f379431
3f512df
c21ed00
52d88a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
[ignore] | ||
# Ignore built/minified addons-frontend code. | ||
<PROJECT_ROOT>/dist/.* | ||
|
||
# These modules opt into Flow but we don't need to check them. | ||
.*/node_modules/babel.* | ||
.*/node_modules/react-nested-status | ||
.*/node_modules/stylelint | ||
|
||
[include] | ||
|
||
[libs] | ||
# TODO: this can go away after | ||
# https://github.com/mozilla/addons-frontend/issues/2092 | ||
./flow/libs/dedent.js.flow | ||
|
||
[options] | ||
# This maps all Sass/SCSS imports to a dummy Flow file to suppress import | ||
# errors. It's not necessary for Flow to analyze Sass/SCSS files. | ||
module.name_mapper.extension='scss' -> '<PROJECT_ROOT>/flow/flowStub.js.flow' | ||
module.system=node | ||
module.system.node.resolve_dirname=node_modules | ||
module.system.node.resolve_dirname=./src | ||
log.file=./flow/logs/flow.log | ||
suppress_comment= \\(.\\|\n\\)*\\$FLOW_FIXME | ||
suppress_comment= \\(.\\|\n\\)*\\$FLOW_IGNORE |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
logs | ||
*.log | ||
npm-debug.log* | ||
flow/logs/*log* | ||
|
||
# Runtime data | ||
pids | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,8 @@ Generic scripts that don't need env vars. Use these for development: | |
| npm run dev:amo | Starts the dev server and proxy (amo) | | ||
| npm run dev:amo:no-proxy| Starts the dev server without proxy (amo) | | ||
| npm run dev:disco | Starts the dev server (discovery pane) | | ||
| npm run flow:check | Check for Flow errors and exit | | ||
| npm run flow:dev | Continuously check for Flow errors | | ||
| npm run eslint | Lints the JS | | ||
| npm run stylelint | Lints the SCSS | | ||
| npm run lint | Runs all the JS + SCSS linters | | ||
|
@@ -84,6 +86,70 @@ or have `InfoDialog` in their behavior text. | |
Any option after the double dash (`--`) gets sent to `mocha`. Check out | ||
[mocha's usage](https://mochajs.org/#usage) for ideas. | ||
|
||
### Flow | ||
|
||
There is limited support for using [Flow](https://flowtype.org/) | ||
to check for problems in the source code. | ||
|
||
To check for Flow issues during development while you edit files, run: | ||
|
||
npm run flow:dev | ||
|
||
If you are new to working with Flow, here are some tips: | ||
* Check out the [getting started](https://flow.org/en/docs/getting-started/) guide. | ||
* Read through the [web-ext guide](https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#check-for-flow-errors) | ||
for hints on how to solve common Flow errors. | ||
|
||
To add flow coverage to a source file, put a `/* @flow */` comment at the top. | ||
The more source files you can opt into Flow, the better. | ||
|
||
Here is our Flow manifesto: | ||
|
||
* We use Flow to **declare the intention of our code** and help others | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These docs are really awesome now, thanks so much for doing another pass. Super easy to grok and the links are great 👍 |
||
refactor it with confidence. | ||
Flow also makes it easier to catch mistakes before spending hours in a debugger | ||
trying to find out what happened. | ||
* Avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) | ||
for any *internal* code. Just declare a | ||
[type alias](https://flowtype.org/en/docs/types/aliases/) next to the code | ||
where it's used and | ||
[export/import](https://flow.org/en/docs/types/modules/) it like any other object. | ||
* Never import a real JS object just to reference its type. Make a type alias | ||
and import that instead. | ||
* Never add more type annotations than you need. Flow is really good at | ||
inferring types from standard JS code; it will tell you | ||
when you need to add explicit annotations. | ||
* When a function like `getAllAddons` takes object arguments, call its | ||
type object `GetAllAddonsParams`. Example: | ||
|
||
````js | ||
type GetAllAddonsParams = {| | ||
categoryId: number, | ||
|}; | ||
|
||
function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { | ||
... | ||
} | ||
```` | ||
|
||
* Use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) | ||
via the pipe syntax (`{| key: ... |}`) when possible. Sometimes the | ||
spread operator triggers an error like | ||
'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/util`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/util.js) | ||
if you have to. This is meant as a working replacement for | ||
[$Exact<T>](https://flow.org/en/docs/types/utilities/#toc-exact). | ||
* Try to avoid loose types like `Object` or `any` but feel free to use | ||
them if you are spending too much time declaring types that depend on other | ||
types that depend on other types, and so on. | ||
* You can add a `$FLOW_FIXME` comment to skip a Flow check if you run | ||
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. | ||
|
||
### Code coverage | ||
|
||
The `npm run unittest` command generates a report of how well the unit tests | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
// This file is just a stub that will suppress 'missing file' errors. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
The files in this directory (when declared in `[libs]` of `.flowconfig`) | ||
will declare global objects. These declarations should be a last resort. | ||
Try to export/import types in the actual source code first. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// TODO: this can go away after | ||
// https://github.com/mozilla/addons-frontend/issues/2092 | ||
|
||
// I'm not sure exactly what this should be. I was using this issue as a guide. | ||
// https://github.com/facebook/flow/issues/2616#issuecomment-289257544 | ||
declare function dedent(params: Array<*>): string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dedent is getting removed so we should remove it and then it doesn't matter. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Flow server logs are written to this directory. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
"dev:amo:no-proxy": "better-npm-run dev:amo:no-proxy", | ||
"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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forget, is this safe to run if you don't have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it's safe. "In addition to the shell's pre-existing PATH, npm run adds |
||
"stylelint": "stylelint --syntax scss **/*.scss", | ||
"lint": "npm run eslint && npm run stylelint", | ||
"servertest": "bin/config-check.js && ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:amo && better-npm-run servertest:disco && better-npm-run servertest:admin", | ||
|
@@ -118,7 +120,7 @@ | |
} | ||
}, | ||
"test": { | ||
"command": "npm run version-check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint", | ||
"command": "npm run version-check && npm run flow:check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint", | ||
"env": { | ||
"NODE_PATH": "./:./src", | ||
"NODE_ENV": "test" | ||
|
@@ -229,6 +231,7 @@ | |
"babel-plugin-react-transform": "2.0.2", | ||
"babel-plugin-transform-class-properties": "6.18.0", | ||
"babel-plugin-transform-decorators-legacy": "1.3.4", | ||
"babel-plugin-transform-flow-strip-types": "6.22.0", | ||
"babel-plugin-transform-object-rest-spread": "6.20.2", | ||
"babel-preset-es2015": "6.24.0", | ||
"babel-preset-react": "6.16.0", | ||
|
@@ -238,6 +241,7 @@ | |
"chai": "3.5.0", | ||
"chalk": "1.1.3", | ||
"cheerio": "0.22.0", | ||
"chokidar-cli": "1.2.0", | ||
"concurrently": "3.4.0", | ||
"cookie": "0.3.1", | ||
"css-loader": "0.28.0", | ||
|
@@ -249,6 +253,7 @@ | |
"eslint-plugin-react": "6.10.3", | ||
"fetch-mock": "5.9.4", | ||
"file-loader": "0.10.1", | ||
"flow-bin": "0.38.0", | ||
"glob": "7.1.1", | ||
"http-proxy": "1.16.2", | ||
"json-loader": "0.5.4", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frustrating that you can't link to the docs without a locale and let it pick up the user's locale. 😒