Add Flow type defenition

[leebyron]

Hell yes

[jasonkuhrt]

@leebyron Curious, I can only assume you knew about flow before us haha, why did you go with TS anyways? Flow just wasn't/isn't as mature?

[leebyron]

TypeScript predates Flow by a while, so when I started this project Flow didn't yet exist. Also, the type definitions are not here for the benefit of the library, but for the benefit of people using the library with these type checking tools. Since a lot of people use TypeScript (myself included) it felt like the right thing to do to include type definition files for it.

Once I get the Flow type definition files, the TypeScript ones will remain in the repository and up to date as well!

[jasonkuhrt]

@leebyron sweet

[lambdahands]

@leebyron We had a discusson on #flowtype – wanted to add the definitions file I was working on, which can be found in my fork here.

Would love some feedback and tips on how to treat something like this. I'm really excited to begin implementing Flow into javascript projects that utilize immutable data.

Cheers!

[leebyron]

Sorry for the delay. I'm excited to see progress on this. Nice work.

How have your flow definitions been working? Are there any holes?

[lambdahands]

Hey, no problem! There are some snags I've run into. I'm not fully acquainted with optional function arguments and how to operate on them in Flow.

Immutable's map and reduce functions use optionals that produce Flow errors; this is the type of code I would typically write as well: https://gist.github.com/lambdahands/3a1caaec0a9eb967b60e

[tmcw]

Once I get the Flow type definition files, the TypeScript ones will remain in the repository and up to date as well!

Are you planning on updating the two manually, or is there some Flow/TypeScript syncing/conversion magic that you can employ?

[leebyron]

The flow team is working on the magic auto-conversion, but it's possible that it will be lossy, so the plan will be to update both manually.

[jareware]

I'm also very excited about being able to combine Flow and Immutable, but I'm in a similar place with this guy.

Do you guys have opinions on how - if at all - this could work out?

Thanks a lot!

[samwgoldman]

I find I use a very small subset of the functionality in this library, so it's not too onerous to maintain my own declaration library, all gratuitously copied from @lambdahands.

[almost]

Any progress on this? I've started with @lambdahands's file (thanks!) but would be great if it was an officially supported thing

[karelbilek]

+1

[arypurnomoz]

+2

[pluma]

+n

[yornaath]

+11

[kastermester]

I have taken a stab at trying to improve the work by @lambdahands. Mainly I added support for the static methods to construct immutables. I also addressed an issue with the static of method on Seq, for some reason it seems to conflict with flow that the return type of the method changes in the classes: Seq, IndexedSeq and KeyedSeq.

I have not tested the definition too much so far, but would love some feedback for fixes. I know I have omitted the usual constructor definitions, but it should be easy to add them back in if wanted.

https://gist.github.com/kastermester/ad79da3e48064effd82e

[ghost]

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

[gasi]

:+1: for using Flow with Immutable.js :smile:

[cpojer]

It's dangerous to go alone! This is the version we have internally at Facebook right now but it is experimental and we don't support it officially yet: https://gist.github.com/cpojer/2dd5e3f4ce41b91a6bd8

The flow team is currently working on improving support for projects like immutable JS so we should have an officially support version of the definition file "soon".

Feel free to fork and contribute to the gist above and let me know how it works out for you. If it is valuable to a lot of people, even if it is not perfect, we might consider making it more official. Personally, the way I see it, having a basic definition file is better than not having one at all. In a project of mine I've successfully adopted the file from above and it is good enough. I had to make some modifications to it and I copy-pasted some type definitions: For example, the return value of a List's filter call is Iterable because that is the type it is inherited from based on the library file above. This means that doing list.filter(fn) will return an Iterable<K, V> instead of a List<T>. To fix this I copied the definitions from Iterable to List (and Map etc.) and turned the return values into List instead of Iterable. I haven't included these local modifications in the gist above but I can add them if it is useful.

Put the file in a lib folder and add it to your .flowconfig like this to make sure Flow doesn't visit your node_modules version of immutable and accidentally types it as any:

[libs] 
lib/

[ignore]
.*/node_modules/immutable/.*

To sanity check if it works, try:

var a: List<number> = List.of('string');

and run Flow – it should yell at you :)

[kastermester]

@cpojer very nice, I'll try it out when I get the time :)

[mrmurphy]

WOOOOOO @cpojer Thanks for sharing!!

[mrmurphy]

@cpojer I'm attempting to start something like Typescript's Definitely Typed over here: https://github.com/splodingsocks/FlowTyped

I was on my way to add the file you linked, but I noticed that the copyright notice at the top says Facebook, all rights reserved. Does that mean I shouldn't put it in that repo?

[bouk]

I've updated @cpojer 's gist to use flow interfaces (flow complained about multiple class inheritance for me otherwise): https://gist.github.com/bouk/d29a6cafab0b9b3b1ec6

The lack of dynamic classes in flow such as those generated by Immutable.Record is unfortunate, but would probably require a big overhaul to support

[nikgraf]

Am I'm doing something wrong or is it fine that flow is not detecting an error with this code:

var b: Map<string, number> = Map({'key': 'value'});

/cc @bouk

[jergason]

Your value isn't a number. That should be an error, so if flow is yelling at you that seems right.

[msolli]

I'm using @bouk's interface file and the following config:

[ignore]
.*/node_modules/immutable/.*

[libs]
.*/src/main/javascript/lib/interfaces/.*

Given this JS file:

/* @flow */
import {List} from "immutable";
var a: List<number> = List.of(666);

I get this Flow error:

/Users/<...>/foo.js:2:9,12: List
This type is incompatible with
/Users/<...>/foo.js:2:9,12: List

/Users/<...>/foo.js:4:23,34: call of method of
Error:
/Users/<...>/foo.js:2:9,12: List
This type is incompatible with
/Users/<...>/foo.js:2:9,12: List

Any idea what's going on?

[nikgraf]

@jergason sorry, I got the text wrong and corrected it above. In the case above Flow didn't point out an error.

[jasonkuhrt]

@msolli I am unable to reproduce your error. The following typechecks for me:

const a : List<number> = List.of(666)

@bouk Thanks, I experienced the same problem and your version seems to work at least initially. I do not know enough about Flow yet to understand what the significance between class and interface is right now.

@jergason @nikgraf I was able to reproduce your issue. I do not understand enough Flow to have a solution. Perhaps of interest however is that this works as expected:

const dict1 : Map<string, string> = Map()
const dict2 = dict1.set('k1', 1) // call of method set Error: number This type is incompatible with string

[bouk]

@jasonkuhrt @jergason @nikgraf weirdly this seems to be a problem where flow chooses to use the empty constructor signature instead of the right one, fixed here: https://gist.github.com/bouk/d29a6cafab0b9b3b1ec6/revisions which now makes

let b: Immutable.Map<string, number> = Immutable.Map({a: 'a'});

fail with 'string This type is incompatible with number'

[bouk]

Maybe someone more familiar with flow (@int3, @mroch) could help us out with understanding why this happens? Flow seems to be picking the empty constructor signature unless I explicitely reorder them, you can see the change here: https://gist.github.com/bouk/d29a6cafab0b9b3b1ec6/revisions

[jasonkuhrt]

@bouk Much appreciated but even with the revised Immutable Type File I receive the same behaviour as before. What version of Flow / Immutable are you seeing your successful results on?

[samwgoldman]

@bouk Flow represents the overloaded constructor signature as an intersection type, where each branch of the intersection is a function type. When checking for errors, Flow will try each branch of the intersection type in turn, until either a suitable type is found or, if no suitable type is found, will add an error.

Because it is not a runtime error to pass overmany arguments to a function, it seems that Flow considers the empty constructor a suitable type and continues. In general, it's important to define overrides from more specific to less specific, but to do so it helps be familiar with the type checking rules. Does that make sense?

[bouk]

@samwgoldman yep that's a great explanation, thanks!

[jasonkuhrt]

Because it is not a runtime error to pass overmany arguments to a function, it seems that Flow considers the empty constructor a suitable type and continues

@samwgoldman @bouk Clear yes, but the answer is quite disconcerting if it means that a constructor type that specifies no arguments effectively allows any number arguments of any type... Am I misunderstanding something? And if not, is this rationalized as an intended design or a feature gap in Flowtype?

And then regarding this problem (Map({ blah: 'blah' }) where does that leave it?

–– Edit: Sorry this conversation is basically off-topic now.

[samwgoldman]

@jasonkuhrt It's not a bug in Flow, just a nuance with how intersection types work (by design) and function types (documented behavior). @bouk's revision seems to fix the libdef, although I haven't tested it myself. Can you provide a complete and minimal repro for the problem you referenced, with expected and actual output?

[jasonkuhrt]

@samwgoldman Thanks for the patience and clarification.

Regarding repro: This should not type check but it does (for me, that's the issue):

lib/test.js

/* @flow */

import { Map } from 'immutable'

const b : Map<string, number> = Map({ a : 'a' }) // Should not typecheck
console.log(b)

.flowconfig

[ignore]
.*/node_modules/immutable/.*

[include]

[libs]
types/

[options]

types/immutable.js

...

declare class Map<K, V> extends KeyedCollection<K, V> {

    static <K, V>(iter: KeyedIterable<K, V>): Map<K, V>;
    static <K, V>(iter: Iterable<any, /*[K,V]*/Array<any>>): Map<K, V>;
    static <K, V>(array: Array</*[K,V]*/Array<any>>): Map<K, V>;
    static <V>(obj: {[key: string]: V}): Map<string, V>;
    static <K, V>(iterator: Iterator</*[K,V]*/Array<any>>): Map<K, V>;
    static <K, V>(iterable: /*Iterable<[K,V]>*/Object): Map<K, V>;
    static <K, V>(): Map<K, V>;

...

[samwgoldman]

OK, so I was able to test this, and the issue is with the last two overloads:

static <K, V>(iterable: /*Iterable<[K,V]>*/Object): Map<K, V>;

This is allowing the object through because the declaration is just wrong—the comment hints at what it should be. If you take this by itself, the declaration says "give me any object, and I'll give you a Map with the key/value types you want" which will allow the invalid call & cast from your example. We can easily fix this by replacing Object with the correct type that is in the comment. Perhaps TypeScript doesn't support that type, but Flow does.

static <K, V>(): Map<K, V>;

This one is trickier. By itself, the "extra arguments to functions are OK" rule is reasonable, and reflects JavaScripts actual runtime semantics. (In fact, without this rule, it wouldn't be possible to write code like xs.map(x => x * 2), because the argument to Array.map has two parameters.) However, here this rule is biting us. Because the call from your example didn't match the Map(obj) constructor, Flow moved on, trying the rest of the overloads until it hit that one. Flow considers any argument as "extra" but at runtime, Immutable will treat the argument as an object and construct a map with initial values—not what we want from a type declaration.

I was able to fix the declaration to work by being clever, but I think this is a usability bug (not a correctness bug), so I'm also thinking about other ways to fix.

    static <K, V>(iter?: KeyedIterable<K, V>): Map<K, V>;
    static <V>(obj?: {[key: string]: V}): Map<string, V>;
    static <K, V>(iterator?: Iterator<[K,V]>): Map<K, V>;
    static <K, V>(iterable?: Iterable<[K,V]>): Map<K, V>;

I replaced the any and Object declarations with the actual, intended type annotations. I also replaced the "empty" constructor by making every argument optional—this achieves the same outcome, allowing empty Map construction, without allowing mis-typed object/array/keyed-iter arguments. (I also removed the Map(array) constructor, because Flow understands that Array<x> is also Iterable<x>.)

I was able to do this because I have a pretty good understanding of how the type checker works, but we shouldn't expect everyone to have that knowledge. That's why I consider this a usability bug. Will bring this up with the Flow team to see what they think.

Edit: I removed the Iterable<any, [K,V]> signature as well, because I'm not sure what that's supposed to mean. Even in TypeScript, Iterable only has a single type arg. (Edit 2: It seems that Immutable defines its own Iterable interface—which does further muddy the waters here...)

[jasonkuhrt]

@samwgoldman Thanks so much for this round of feedback. I can see that I need to spend some due time with Flowtype before racing ahead with it. So as it stands you mostly solved this particular case save for some remaining uncertainty/ambiguity as noted in the edits? @bouk Thoughts?

[bouk]

@samwgoldman great feedback, I think the core issues stem from basically copy and pasting the type script definitions in immutablejs and using those instead of rewriting them to be appropriate for flow. I might spend some time going through all the definitions and 'flowifying' them

The iterable thing is definitely an issue, I just noticed that the type interface file also contains a redefinition for Iterator, which is not needed because it can just use the standard Javascript one. Is there any way to specify explicitly that we accept anything with the global Iterable interface or is that impossible because of name shadowing? Could be fixed by renaming the immutabe iterable to IIterable or something but that's pretty ugly.

Another issue currently is that Record is essentialy untyped. In immutable you can create a new class by using Record, but right now Flow doesn't have the concept of dynamic classes (I believe). Probably a thougher nut to crack.

Thanks for the help

[msolli]

Sorry @jasonkuhrt, I'd stupidly excluded every package.json file in my project, which created a whole bunch of strange problems. All solved now. Thanks for looking into it. Excited to see where this discussion is going!

[jasonkuhrt]

@msolli No problem! Glad you worked it out.

[thomas-jeepe]

The way that Immutable Maps work is with Array types, are there any plans to change it or is that the way it is designed internally?

Also, any updates? Using it right now and it seems pretty good :)

[cythrawll]

With movement of interfaces from IndexedSeq to Seq.Indexed, etc... the type definition files provided here by @cpojer are completely broken. Can't for the life of me figure out how to update the file either. How do you nest interfaces that way?

[AlexGalays]

Looks like Facebook either doesn't use immutable-js, flow, or both ? :P

[cpojer]

This is the latest version that we are using: https://gist.github.com/cpojer/3a2aad89499c37fbb2db cc @gabelevi @jeffmo

[AlexGalays]

Nice

[bouk]

@cpojer @AlexGalays @jeffmo @gabelevi I made a small modification to make Record a bunch more useful. It now returns typeof Map<string, any> instead of any so that flow will actually check that you're calling methods that exist. The modified version can be found here: https://gist.github.com/bouk/911b8d4993c19de3f98f

One side effect is that flow now will also check that all the properties exist, so you will need to define those manually through extending the Record, like so:

class ABRecord extends Record({a: 1, b: 'etc.'}) {
  a: number;
  b: string;
}

Seems to work almost perfectly, except that because it extends Map<string, any> the return type of any of the methods is also Map<string, any>. This means that the returned value is missing the properties and methods of the extended class, so it breaks down pretty clearly.

This could be fixed by having some sort of Self type name in interface files that refers to the extended class. So that if the definition of set on Record$Class was set(key: string, value: any): Self then new ABRecord().set('a', 1) would still have the type ABRecord instead of Record$Class.

[arunabha]

@bouk Thanks for providing the flow definitions for immutable !

For the following code

import type {List} from "immutable";
const a : List<number> = List.of(666);

I'm getting

 39: const a : List<number> = List.of(666);
                              ^^^^ List. type referenced from value position
 15: import type {List} from "immutable";
                  ^^^^ type List

Relevant parts of .flowconfig are

[ignore]
.*/node_modules/immutable/.*

[libs]
lib/

where lib has the immutable.js from the gist.

I can see the definition of 'of' in the definitions, so I'm wondering why flow can't see that List.of is the use of the 'of' method.

[mhagmajer]

You need to import the actual module whereas in your code you're just importing the type. Try this

import { List } from 'immutable';

On Sat, Dec 12, 2015, 12:41 Arunabha Ghosh notifications@github.com wrote:

@bouk https://github.com/bouk Thanks for providing the flow definitions for immutable !

For the following code

import type {List} from "immutable";const a : List = List.of(666);

I'm getting

39: const a : List = List.of(666); ^^^^ List. type referenced from value position 15: import type {List} from "immutable"; ^^^^ type List

Relevant parts of .flowconfig are

[ignore] ._/nodemodules/immutable/.

[libs] lib/

where lib has the immutable.js from the gist.

I can see the definition of 'of' in the definitions, so I'm wondering why flow can't see that List.of is the use of the 'of' method.

— Reply to this email directly or view it on GitHub https://github.com/facebook/immutable-js/issues/203#issuecomment-164140531 .