Improve Flow libdefs

[marudor]

filter is no longer declared on Iterable.
This is to degrade the Type of a Iterable to any. Mainly to allow following:

function foo(): List<number> {
   return List([undefined, 1, 2]).filter(i => i); //List no longer contains undefined. Without degrade to generic List it would error.
}

flatten is also declared on each Class to return the "correct" type. Before Iterable was returned. This results in missing chaining.

List().flatten().map() // Errored before as map is not defined on Iterable

get no longer returns ?T but T.
Technically it is not correct, but it allows common use cases and is in sync with the typescript libdef.

Also fixed bugs with get introducing undefined into the Type of a Iterable. (Ordering of overloaded function declarations is important)

[leebyron]

shouldn't push and unshift have similar type definitions?

[leebyron]

What's different about filter and flatten that need to be defined differently from say, map, which is defined directly on Iterable?

[leebyron]

perhaps make this an explicit List<any> so it's clear that this was moved here to define use of <any>?

[marudor]

Included the where needed.

[marudor]

The typo with push is fixed.
map is actually not defined on Iterable.

For filter and flatten I've included examples in the PR description.
Filter can change the Types that an Iterable includes. The most important one being undefined.
filter can change a type from ?T to T. That's often quite important.
I degrade it to a non specific Iterable (Or Map, List, Set and so on)

Same for flatten.
It can change the Types of the Iterable. But we can't really see or know if they do. So we degrade and leave it to the user to know it.

[leebyron]

Not sure it's the right thing to have filter drop the type parameter though.

Based on flow's treatment of Array#filter: https://github.com/facebook/flow/blob/master/lib/core.js#L185 - we should probably match it.

[marudor]

Well - the filter treatment in flow core is flawed in the same way.
Imagine:

const arr = [1, 'foo', 2, 'bar', 3, 'batz'];
const numberArr: number[] = arr.filter(x => Number.isInteger(x)); // Error, string is not compatible with number
const stringArr: string[] = arr.filter(x => typeof x === 'string'); // Error, number is not compatible with string

Actual results of both filter match the type. Flow doesn't know that though. It still assumes Array<number | string>;

[leebyron]

Right - but at least we could be consistent with flow's existing behavior rather than divergent.

I'd argue that both behaviors are non-ideal, but it usually makes me nervous to drop type information if it's not strictly necessary, that can result in false-negatives.

Example:

const arr = [1, 'foo', 2, 'bar', 3, 'batz'];
const numberArr = arr.filter(x => typeof x === 'number');
numberArr[0].toLowerCase(); // Wait, where's my error?!

[leebyron]

It's reasonably common practice in flow where filter is used to filter out types rather than values to take type coercion into your own hands to make it clear what you're doing:

const arr = [1, 'foo', 2, 'bar', 3, 'batz'];
const numberArr: Array<number> = arr.filter(x => typeof x === 'number'); // Error
const numberArr: Array<number> = (arr.filter(x => typeof x === 'number'): any); // No Error

[marudor]

I would rather degrade I think.
But to match flow core I think changing it to this again should work for now.
This is probably more a question for flow and the way the main lib should go.
We should not introduce different behavior for essentially the same function.

[leebyron]

should this still have <T>?

[marudor]

Not needed - optional. As the List itself already has T we do not need to bind a T to the function.

[leebyron]

should other static methods have the same style then? when do we use <T> on a static vs not use it?

[marudor]

We need it when we bind another dynamic type.

declare class Foo<T> {
 static (param: T): Foo<T>; //not needed.
 static <T_>(param: T, cb: (p: T, extra: T_) => T_): Foo<T> // needed - but only T_ not T.
}

The TypeParam of the class we define the static on is implicitly available.

[leebyron]

Gotcha, was just noticing static of<T> right below this one - was just curious why you decided to edit this in this diff, but not any others like it.

[marudor]

Probably because I edited that constructor in my edits and just did not edit the other ones.
Although I ended up with exactly the same constructor I still rewrote it.
Gonna remove them everywhere.

[leebyron]

No sweat - just my curiosity!

[leebyron]

Thanks again for your help improving these