First flow types; port external links editor tests to Selenium #550
Conversation
mwiencek
force-pushed
the
flow
branch
4 times, most recently
from
85b379e
to
514d06e
Compare
.flowconfig
Outdated
| .*/root/static/build/.* | ||
| .*/t/.* | ||
|
|
||
| [include] | ||
|
|
||
| [libs] | ||
| ./node_modules/jsx-control-statements/jsx-control-statements.latest.flow.js |
There was a problem hiding this comment.
Using npm 3.10.10, I have ./node_modules/jsx-control-statements/jsx-control-statements.flow.js instead, resulting into the following message in flow 0.55.0 logs:
Skipping ./node_modules/jsx-control-statements/jsx-control-statements.latest.flow.js: No such file or directory
There was a problem hiding this comment.
Huh, guess I didn't git add the package.json changes. It should be fixed now if you run npm install again.
There was a problem hiding this comment.
(I'd also recommend doing npm install -g npm@latest if you can though, the latest versions of npm seem much less flaky/buggy for me.)
Point to the types supported by the latest version of flow.
Replaces PropTypes with flow types in the two modules that use those, adding types to the entire files as needed. It's currently a pain to type-check immutable-js records, so I removed those from the external links editor source in favor of plain objects.
This is causing tests to fail in the test container, though they pass fine locally.
This only seems to be necessary on our slow Jenkins server.
| @@ -91,15 +108,15 @@ class ExternalLinksEditor extends React.Component { | |||
| } | |||
|
|
|||
| getOldLinksHash() { | |||
There was a problem hiding this comment.
Shall we check return type (similarly to parameters type)?
There was a problem hiding this comment.
Good point actually. Flow has type inference, so normally we don't have to specify types where it can figure them out on its own. But for external modules, it says this:
If a third-party library that has no type information is used by your project, Flow will treat it like any other untyped dependency and mark all of its exports as
any. Interestingly, this is the only place that Flow will implicitly injectanyinto your program.
Since we're returning the result of a lodash function, I believe the inferred return type is any, which disables type checking wherever the function is used.
So adding return types is a good idea wherever we're calling an external library function and returning its result. A better idea, if possible, is pulling in types for the library via flow-typed (mentioned in the link above), and letting flow infer the types from that.
There was a problem hiding this comment.
flow-typed only has types for lodash v4, so I'd prefer to do that in a later PR.
There was a problem hiding this comment.
I started to tickle updating to lodash v4 alredy.
This begins adding static type checking to our JavaScript with Flow: https://flow.org/
Only files beginning with a
// @flowcomment are checked, so this can be done incrementally.The types aren't yet checked during builds. I'll do that in a future PR, but for now devs are expected to run
flowon their own.The types are stripped via a babel plugin.
This PR also ports the external links editor tests to Selenium, because I had trouble getting the finicky JS ones to pass consistently.