Flow: Enforce 100% coverage

[steida]

// @flow-strict

Why? Because there are many cases where 100% type coverage needs a lot of type boilerplate, but without it, Flow somehow still detects wrong types, but only coverage is smaller. Enforcing 100% coverage would help I suppose.

[STRML]

I agree, this is easy to mess up; Flow tends to silently ignore types it can't figure out and it can be tough to tell what has been missed. It helps very much to use an editor plugin that shows coverage, but it should be possible to do this well via the CLI or configuration as well.

[steida]

All we need right now is flow strict mode directive because it's easy to break something without warning.

[agentcooper]

100% is sometimes hard to reach, however coverage regressions would be nice indeed.

I think this can be implemented as a third-party solution. A pre-commit script could run coverage on all // @flow files, record the results and compare them with the previous stats. I started https://github.com/agentcooper/flow-stats with this in mind, but did not finish it.

[nickmessing]

@steida, there are still things that you cannot annotate:

try {

} catch (e) {} //you cannot annotate e here ever

export default {
  myMethod () {
    this.body = 3 //you cannot annotate this
  }
}

I use second structure with koa@2 and pass ctx as context into the methods as in koa@1, I've seen no way to annotate it other than const ctx: Context = this

[steida]

@nickmessing That's fine. The purpose is different. Sometimes we have 100% coverage only because of type inference. When something is broken, coverage goes down without error. // @flow strict could detect it.

cc @calebmer

[calebmer]

I’m glad to hear that 100% coverage is desirable 😊

We encourage you to get coverage information from Flow and add your own rules to stop the build if coverage isn’t satisfactory. To get coverage information run:

flow coverage --json <file>

You can see Nuclide use it here: https://github.com/facebook/nuclide/blob/9a380e8c605dd68a8b70ed75cf655629c185166b/pkg/nuclide-flow-rpc/lib/FlowSingleProjectLanguageService.js#L424

[steida]

Simple stupid // @flow 100% or // @flow 98% would be more explicit without a need to implement anything else, but I understand it has no priority so feel free to close it.

[k]

I agree with @steida 100% flow coverage is desirable. I, however, cannot enforce 100% flow coverage completely because of two reasons currently

1. compose function itself from recompose seems to always be typed as any even though it types whats inside it. I believe this is a bug with how flow interprets $Compose
2. Exceptions as mentioned above by @nickmessing.

A current workaround could be writing a custom script that checks all changed files and looks in a JSON file which maps thresholds for specific files. But that can easily be abused.

[LoganBarnett]

@k @nickmessing right now we have an untyped.js file that we exclude from the flow-coverage-report. In it is a helper that gets us out of needing to use try/catch everywhere:

export function coerceThrowToReturn<T> (f: () => T): T | Error {
  try {
    return f()
  } catch (untypedError) {
    if (untypedError instanceof Error) {
      return untypedError
    } else {
      return new Error(untypedError)
    }
  }
}

Then we have to do a result instanceof Error check before we can use the return result of f.

So far, that's literally all we've needed to achieve 100% type coverage, and we enforce that with flow-coverage-report. https://github.com/rpl/flow-coverage-report

[svsool]

@k I guess compose was typed as any because of this problem https://github.com/flowtype/flow-typed/pull/1563 at least when you use it as import compose from 'recompose/compose' and not as import { compose } from 'recompose'

[k]

@svsool I tried importing both ways and have the same problem. The the internals of the compose are typed but flow-coverage thinks the compose function itself is untyped

@LoganBarnett good idea! I will use that trick once we figure out a solution to the untyped compose function.

[LoganBarnett]

@k https://github.com/LoganBarnett/error-as-either also includes the types in a place Flow can find. It's basically a packaged version of the function in my earlier comment. Then you don't need to make exceptions in your coverage report.

[MoOx]

Something like jest coverageThreshold would be really nice to have in order to trigger a error code at the end of the flow command (would be handy for CIs) https://facebook.github.io/jest/docs/en/configuration.html#coveragethreshold-object

[MoOx]

I guess addressing #2981 would cover this issue

[mgtitimoli]

Until we get natively support we can use eslint-plugin-flowtype-errors/enforce-min-coverage rule.

[szagi3891]

I advise you to switch to TypeScript. I used Flowtype for 2 years. TypeScript I use for about 2 months. Typescript is much better than flowtype.
There is no problem with the fact that there is something uncovered (in strict mode).