Flow: Can't get flow type coverage for a function

[kasperpeulen]

I have the following function:

type KeyboardInfo = {
  key?: string,
  code?: string,
  meta?: boolean,
  alt?: boolean,
  ctrl?: boolean,
  shift?: boolean,
  prevent?: boolean,
  log?: boolean
}

function on(event: KeyboardEventTypes, {key, meta, alt, code, shift, ctrl, prevent = true, log = false}: KeyboardInfo, callback: (e: KeyboardEvent) => void) {
  window.addEventListener(event, (e: KeyboardEvent) => {
    if (log) {
      console.log(e);
    }

    if (key && e.key !== key) {
      return;
    }
    if (meta && e.metaKey !== meta) {
      return;
    }
    if (code && e.code !== code) {
      return;
    }
    if (ctrl && e.ctrlKey !== ctrl) {
      return;
    }
    if (alt && e.altKey !== alt) {
      return;
    }
    if (shift && e.shiftKey !== shift) {
      return;
    }
    if (prevent) {
      e.preventDefault();
    }
    callback(e);
  });
}

I try to get this covered by flow, but I'm not sure how. Atom says it is not covered:

Is this because I call window? Not sure what is going on here.

[rpominov]

Maybe Flow wants you to specify return type of the callback?

- window.addEventListener(event, (e: KeyboardEvent) => {
+ window.addEventListener(event, (e: KeyboardEvent): void => {

Another possibility is that it just points out that window.addEventListener is uncovered by built in type definitions (https://github.com/facebook/flow/blob/master/lib/dom.js).

Or maybe both. Anyway those are just guesses, but I thought it might be helpful.

[kasperpeulen]

Another possibility is that it just points out that window.addEventListener is uncovered by built in type definitions (https://github.com/facebook/flow/blob/master/lib/dom.js).

Seems like this is the case. Actually the whole window object seems to be missing ....

[aackerman]

@kasperpeulen it's often useful to use the flow cli to get better information.

The --color flag, which seems to be undocumented will mark exactly the issues.

[kasperpeulen]

@aackerman Thanks for the tip 👍

@avikchaudhuri Is there any reason that the window object is not typed yet?

I saw typescript has some type definition here:
https://github.com/Microsoft/TypeScript/blob/master/src/lib/dom.generated.d.ts#L12835

[avikchaudhuri]

@kasperpeulen No good reason. Would you like to send a PR?

[kmiyashiro]

Is there a way to convert the TypeScript type to Flow type? That generated window type is huge.

[nmn]

@kmiyashiro Not yet no. But a lot of the syntax is similar so you can usually start by copying the TS file and manually editing it till it works. Remember to add optionals where needed.

[LoganBarnett]

@avikchaudhuri Is there a place we should put this? It seems like it would be easy to make it a global libdef (is it very important that Flow knows it's in a browser or hybrid environment?). Then we'd provide it via flow-typed. However, flow-typed seems like it's for specific libs and having a global libdef might be out of place. If we produce a PR internally, can we still provide a .flow.js libdef for it? I'm not familiar with how the builtin types get expressed.

[asolove]

Curious if there's further interest in adding a window type. Seems like we are unlikely to get an exact type for it.

[jfirebaugh]

I've added one locally for a project I work on. I would love to see something like it in flow core.

[LoganBarnett]

@asolove I haven't looked at the Typescript declaration but I have no doubt it's non-trivial in size. I'd be up for helping with incremental PRs.

[asolove]

The only problem with incrementalism is that a partial window declaration will break existing code that works because of the any type.

[LoganBarnett]

I don't understand, and would like to :)

Any code that uses window right now cannot type check unless someone shims a libdef in, correct? Or is this more for when people hang things off of window as a means of adding globals?

[asolove]

Right now flow declares window this way:

declare var window: any;

So all uses of window typecheck correctly, although also aren't considered covered for the coverage report.

Replacing that with a definition of window that doesn't include every possible browser api would cause currently passing code to fail to type check. So I doubt that is a workable solution.

Would be nice to have a way to opt in to a more carefully typed window object though.

[kmiyashiro]

@asolove how does typescript get around this issue with their pre-defined window type? I believe they use Edge's API as the basis for their type, which most likely overlaps with the vast majority of modern browsers.

[asolove]

Good point, wonder if we can piggyback on what they do.

[asolove]

So it looks like TS has a built in way to declare new stuff that hangs off the window object. As a result, some window typings are distributed like libraries for each chunk of functionality like web audio. Which means there can be different
versions, you can write them yourself to use in your project, etc.

This probably won't work in Flow, since I don't think you can extend interfaces from another file. Maybe window should b a special case?

[LoganBarnett]

Perhaps it would be better to accommodate Javascript's mutable nature and allow any type/interface to be "patched". This way when libs want to hang things off of window, we can go into the libdef and provide an additive annotation to it.