Type definitions for use with Flow

[hallettj]

This pull request includes type definitions, and adds a step to the build process to copy the type definitions to dist/. With the definitions file in dist/, users who import Immutable and who use Flow will pick up the type definitions automatically.

This is intended to address issue #203.

An important caveat when using these definitions is that the types for Iterable.Keyed, Iterable.Indexed, Seq.Keyed, and so on are stubs. When referring to those types, you can get the proper definitions by importing the types KeyedIterable, IndexedIterable, KeyedSeq, etc. For example,

import { Seq } from 'immutable'
import type { IndexedIterable, IndexedSeq } from 'immutable'

const someSeq: IndexedSeq<number> = Seq.Indexed.of(1, 2, 3)

function takesASeq<T, TS: IndexedIterable<T>>(iter: TS): TS {
  return iter.butLast()
}

takesASeq(someSeq)

This is because I have not been able to work around issues Flow has with a class holding a reference to its own subclasses, and with overriding static properties with values that have different types. (E.g., Seq.Keyed is not the same type as Iterable.Keyed, but Seq is a subclass of Iterable.)

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[tsing]

+1 This is very useful.
Btw, I thinks you could also add a flow type file for contrib/cursor

[chrisui]

@hallettj Keen to get flow working in our codebase at work so I took this for a spin as it's looking promising judging by conversation over at #203.

Hit a blocker straight away though with the Record type. The following doesn't type check:

import {Record} from 'immutable';

const Project = Record({
    id: undefined
});

const myProject = new Project({
    id: 1
});

will give the error:

test.js:7
  7: const Project = Record({
                     ^ function call
 12:    id: 1
            ^ number. This type is incompatible with
  8:    id: undefined
            ^^^^^^^^^ undefined

Am I missing something extra to get flow working here? I saw the paragraph in #203 about Records but it seems to me like the above (standard way of using records) should still work alongside being abl to generate a stricter type with the desired record shape?

I guess this highlights another requirement for this PR... Docs! :)

I will update this with any other findings I might make.

[samwgoldman]

@chrisui Does this work?

const Project = Record({
    id: (undefined: number|void)
});

[chrisui]

@samwgoldman yup!

So many nifty little tricks hidden away with flow! (okay they may not be "hidden", that "nifty" or "tricks" but to me they are! :))

[chrisui]

@samwgoldman Is there any way we can support the following?

import {Record} from 'immutable';

function createApiRecord(values) {
  const NewRecord = Record(values);

  return class ApiRecord extends NewRecord {
    static TYPE = values.type;
  };
}

const Project = createApiRecord({
  type: 'project',
  id: (undefined: string|void)
});

const myProj = new Project({
  id: '1'
});

Current errors:

test.js:8
  8:   const NewRecord = Record(values);
                         ^^^^^^^^^^^^^^ function call
 10:   return class ApiRecord extends NewRecord {
                                      ^^^^^^^^^ identifier `NewRecord`. Expected polymorphic type instead of
 10:   return class ApiRecord extends NewRecord {
                                      ^^^^^^^^^ function type

The purpose of this is to provide some introspection to our records. Ie. nicer to do type === Record.TYPE than type === (new Record()).type

[hallettj]

@tsing: Thanks for the feedback! I don't use the cursor feature. (I prefer to use my own lens library.) I'm not inclined to write type definitions for a module that I do not plan to use. But if you or someone else want to add definitions for cursor types, I think that would be done by creating a file contrib/cursor/index.js.flow.

[leebyron]

This is so dope. Thanks for your hard work!

[jareware]

I guess this highlights another requirement for this PR... Docs! :)

Speaking of which... are there any? :) How are Records supposed to work?

For the following file:

// @flow

import { Record } from 'immutable';

const Project = Record({
  price: 123
});
const test = new Project({
  price: /not a number/
});

// I think all of these should be errors:
test.set('price', 'definitely not a number');
console.log(test.doesNotExist);
console.log(test.id.substr(1));

I only get a single error:

$ flow check

immutable-test.js:40
 40:   price: /not a number/
              ^^^^^^^^^^^^^^ RegExp. This type is incompatible with
 36:   price: 123
              ^^^ number

Found 1 error

I've also tried the syntax outlined in #203 (comment), but it doesn't seem to fair any better in catching these issues.

Is Record support just not there yet?

$ flow version
Flow, a static type checker for JavaScript, version 0.23.0
$ npm list | grep immutable
+-- immutable@3.8.0

[MSch]

First off, thank you so much for that work, it is super amazing to have flow definitions in immutable js now!

With the flow definitions that were floating around previously we could define a record as an ES6 class and have flow correctly typecheck it. Now with immutable 3.8.0 and flow 0.23.0 this doesn't work any more.

/* @flow */

import { Record } from 'immutable'

export default class Person extends Record({
  name: null,
  email: null,
}) {
  name: string;
  email: string;
}

  5: export default class Person extends Record({
                                         ^ function call. Expected polymorphic type instead of
  5: export default class Person extends Record({
                                         ^ function type

Can anyone point me in the right direction in order to fix this?

[hallettj]

@jareware: There are notes on the limitations of record type-checking in this comment from the discussion on #203. That is certainly not the best place for that information.

[hallettj]

With the flow definitions that were floating around previously we could define a record as an ES6 class and have flow correctly typecheck it. Now with immutable 3.8.0 and flow 0.23.0 this doesn't work any more.

@MSch : Thanks for the heads-up! I am looking into it.

[MSch]

@hallettj I think this was already fixed in #841 - at least my usage of Records typechecks again

[karelbilek]

Hello, I am a little lost in all the comments and versions.

With the current flow and current head of your fork, how do the Records work and what they check and what they don't check?

Why is, for example, the Record signature

static <T: Object>(spec: T, name?: string): /*T & Record<T>*/any 

? When I uncomment the annotation as

static <T: Object>(spec: T, name?: string): T & Record<T> 

it seems to work better.

[karelbilek]

Also, shouldn't Iterable's (and therefore, Map's) get be like this

get(key: K): ?V;
get<V_>(key: K, notSetValue: V_): V|V_

instead of this

get(key: K): V;
get<V_>(key: K, notSetValue: V_): V|V_

?

Flow's Map has this

get(key: K): V | void;

I can't tell what's the difference between V | void and ?V, it seems the same to me.

[hallettj]

@Runn1ng wrote:

Why is, for example, the Record signature

I put in the T & Record<T> return type, but it was commented out in c28c8ae. Based on the code comments it looks like there are some problems with existing code. Hopefully the type will be changed back, or changed to something more accurate, when those problems are sorted out. I'm guessing that will require updates to Flow.

In the meantime you can get that type by putting explicit annotations in your code:

const MyRecord = Record({ /* whatever structure `MyType` has */ })
const myRecord: MyType & Record<MyType> = new MyRecord({ /* whatever */ })

Btw, @marudor, I love the changes that you made to the type definitions! I'm going to have to remember that trick with type parameters on _Iterable the next time I come across a similar problem of circular references.

Also, shouldn't Iterable's (and therefore, Map's) get be like this

I agree that using ?V as the return type for get would be more accurate. On the other hand it would force undefined checks to an extent that might be more annoying than helpful. There is much precedent for partial function types for data structure access (partial meaning a type that claims to return a value, when there are cases where it might not actually return a value). The choice of using a partial function or forcing a check for undefined is a judgment call. My own preference is to have both options: an unsafe partial function, and a safe version that does the same thing, but that expresses that the return value might be undefined.

I can't tell what's the difference between V | void and ?V, it seems the same to me.

Those types are nearly the same. I believe that ?V is actually a shorthand for V | void | null. In most cases having the null alternative is not a problem, even if the function never actually returns null. I tend to use ?V for brevity, even though it is slightly less precise than V | void.

Edit: My Record example was not actually correct.

[karelbilek]

Thanks for the comments!

There is much precedent for partial function types for data structure access (partial meaning a type that claims to return a value, when there are cases where it might not actually return a value).

Hm. What I do like about Flow is that it doesn't have partial function types and I cannot accidentally have undefined/null by mistake. And I am not sure about the precedent - as I said, in Flow's own Map, the returning type is ?V.

But otherwise the definition file is great, I am using it right now.

[marudor]

I actually talked with @leebyron about the ?V vs V on get.
We decided to go with V (for now)
One reason was to not break existing code.
Previous Immutable libdefs all had V instead of ?V.
And just as @hallettj said it was more annoying than helpful to have all the checks for undefined/null.