Add flow definitions #478
Add flow definitions #478
Conversation
|
Thanks @TrySound! I started looking through this a bit tonight. I should be able to get through all of it over the next few days. |
package.json
Outdated
| "build": "npm run build:dist && npm run build:min && npm run build:flow", | ||
| "build:dist": "rimraf lib dist && buba src -o lib && rollup -c", | ||
| "build:min": "uglifyjs dist/most.js -c \"warnings=false\" -m > dist/most.min.js", | ||
| "build:flow": "cpy type-definitions/most.js.flow dist", |
There was a problem hiding this comment.
It's typical (and useful!) to call this file src/index.js.flow, since that will allow flow to find it automatically while hacking on mostjs itself. We won't get immediate benefit from that, since we don't use types internally in src/ yet, but it would allow us, for example, to start enabling flow in unit tests.
There was a problem hiding this comment.
Moved to src/index.js.flow
type-definitions/most.js.flow
Outdated
| @@ -0,0 +1,410 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
See other comment about naming this file src/index.js.flow.
type-definitions/most.js.flow
Outdated
| // Note: Without higher-kinded types, the types for these | ||
| // cannot be written in a completely safe manner; See https://github.com/Microsoft/TypeScript/issues/1290 | ||
| // For better type safety, consider using most.join/switch/switchLatest with thru | ||
| join(): A; |
There was a problem hiding this comment.
I'm not sure what to do here in flow ... but at the very least, we could remove the TS-specific link in the comment.
There was a problem hiding this comment.
Removed comment and methods
type-definitions/most.js.flow
Outdated
| combineArray<V, R>( | ||
| fn: (a: A, ...rest: V[]) => R, | ||
| streams: Stream<V>[] | ||
| ): Stream<R>; |
There was a problem hiding this comment.
I see that the TS defs use this as well, but I think we should change it. This enforces that all the streams must be of the same type (they must be Stream<V>), which feels maybe too restrictive for a final "catch all" variant.
The combineArray type defs in @most/core use any.
I guess it's a tradeoff: combineArray with > 5 streams is either a) extremely restrictive, allowing you only to combine streams of exactly the same type (this seems unusably restrictive to me!), or b) gives up type safety by allowing any (this seems like a reasonable opt-in behavior to me).
I vote for option b, but want to see what everyone else thinks.
There was a problem hiding this comment.
Did you ever have cases with more then five streams?
There was a problem hiding this comment.
I don't think any is a good case. It will hide output type and skip errors when user trust flow. I saw typings with even ...rest: Array<void>, so I vote for safer case.
There was a problem hiding this comment.
I think Stream<V>[] is probably a bit restrictive. I agree with @briancavalier that Array<Stream<any>> is a slightly better choice.
If we're going to do any modifications to the TypeScript definitions adding a ReadonlyArray<Stream<any>> or a variant of that would be a nice addition
There was a problem hiding this comment.
I'm afraid this will break safety of the library even more, since we mentally trust to definitions.
There was a problem hiding this comment.
The problem is that typescript requires you to specify more types, but flow mostly can infer them and if it does not ask to specify type (it's not exported) then everything should work correctly.
type-definitions/most.js.flow
Outdated
| timestamp(): Stream<TimeValue<A>>; | ||
| delay(dt: number): Stream<A>; | ||
|
|
||
| await<B>(this: Stream<Promise<B>>): Stream<B>; |
There was a problem hiding this comment.
Afaik, using this here is valid in TS, but not in flow. See facebook/flow#452.
Unfortunately, I don't think flow has any way of handling this situation. So, I think we either have to treat await() and awaitPromises() like join(), or omit the method versions from the type definitions, forcing flow users to call the function versions instead.
Thoughts?
type-definitions/most.js.flow
Outdated
| declare export function of<A>(a: A): Stream<A>; | ||
| declare export function empty(): Stream<any>; | ||
| declare export function never(): Stream<any>; | ||
| declare export function from<A>(as: A[] | Iterable<A> | Observable<A>): Stream<A>; |
There was a problem hiding this comment.
At first, I thought A[] isn't valid flow array syntax, but turns out it is! TIL 😄 👍
type-definitions/most.js.flow
Outdated
| declare export function combineArray<V, R> ( | ||
| fn: (...items: V[]) => R, | ||
| items: Stream<V>[] | ||
| ): Stream<R>; |
There was a problem hiding this comment.
Whatever we decide about any vs V for the method form of combineArray, we can apply here, too.
|
I think we can keep array definitions as is and maybe extend them when somebody will leave an issue. |
|
After thinking about this a bit more, I agree, @TrySound. The current flow array defs match the TS defs, for better or for worse, at least they are consistent. It'd also be a breaking change in 1.x to remove the catch-all TS variants, so I think we just go with it in 1.x and think ahead to a better solution for 2.x. |
src/index.js.flow
Outdated
|
|
||
| declare type CreateGenerator<A> = (...args: Array<any>) => Generator<A|Promise<A>, any, any>; | ||
|
|
||
| export interface Sink<A> { |
There was a problem hiding this comment.
One last thing: I'd like to go with types unless there's a compelling reason to prefer interfaces.
We've preferred type over interface in @most/core's flow types. I don't see any benefit to having 2 things, types and interfaces, vs having only types. It'd also nice to be consistent with @most/core, although I'm not too concerned about it.
Otherwise, this all looks good to me.
No description provided.