-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Flow libdefs #845
Improve Flow libdefs #845
Conversation
@@ -400,7 +430,7 @@ declare class List<T> extends IndexedCollection<T> { | |||
remove(index: number): this; | |||
insert<U>(index: number, value: U): List<T|U>; | |||
clear(): this; | |||
push<U>(...values: U[]): List<T|U>; | |||
push<U>(...values: U[]): List<U>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't push and unshift have similar type definitions?
What's different about |
filter( | ||
predicate: (value: T, index: number, iter: this) => mixed, | ||
context?: any | ||
): List; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps make this an explicit List<any>
so it's clear that this was moved here to define use of <any>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included the where needed.
The typo with push is fixed. For filter and flatten I've included examples in the PR description. Same for flatten. |
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. |
Well - the filter treatment in flow core is flawed in the same way. 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>; |
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?! |
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 |
I would rather degrade I think. |
This has some downsides but matches flows libdef for Array.filter. If this is the best way should be discussed on flow. We just match the main libdef for filter.
@@ -390,7 +400,7 @@ declare class SetSeq<T> extends Seq<T,T> mixins SetIterable<T> { | |||
} | |||
|
|||
declare class List<T> extends IndexedCollection<T> { | |||
static <T>(iterable?: ESIterable<T>): List<T>; | |||
static (iterable?: ESIterable<T>): List<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this still have <T>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed - optional. As the List itself already has T we do not need to bind a T to the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should other static methods have the same style then? when do we use <T>
on a static vs not use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sweat - just my curiosity!
Thanks again for your help improving these |
filter is no longer declared on Iterable.
This is to degrade the Type of a Iterable to any. Mainly to allow following:
flatten is also declared on each Class to return the "correct" type. Before Iterable was returned. This results in missing chaining.
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)