Skip to content
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

Apply new technique for strong heterogenous array types #281

Merged
merged 5 commits into from
May 29, 2019

Conversation

briancavalier
Copy link
Member

@briancavalier briancavalier commented May 6, 2019

This uses a variation of the technique from Simpler Variadic Function Types in Typescript to improve the types of zipArray and combineArray for TS, allowing them to scale to any arity.

It also improves the type of mergeArray, allowing it to be heterogeneous while remaining strongly typed. Essentially, this extends the union behavior of merge to arbitrary arrays of streams. This works by leveraging the fact that TS union types obey a distributive law.

Potential Gotchas

In general, I see strong heterogeneous types as a plus. However, there could be a couple downsides:

  1. They could be surprising or confusing for some users.
  2. I haven't found any way to achieve parity in Flow. What are the implications for documentation?

Todos

  • ToStreamsArray is duplicated, consider extracting it somewhere
  • Figure out the implications for docs, given that Flow can't handle this (afaict)
Not Found
Copy link
Member

@Frikki Frikki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using the newer readonly syntax for read-only arrays.

@unscriptable
Copy link
Member

I’m going to try to find some time to investigate this technique in Flow. I’ve been looking for a generic solution for this pattern for a while.

@briancavalier
Copy link
Member Author

I'd love to find at least an approximation in Flow. I tried (very) briefly to get something working with type-level functions (via $Call), but never managed to find something I was really happy with. It'll be great to have someone else give it a go!

@briancavalier
Copy link
Member Author

briancavalier commented May 7, 2019

@unscriptable Here's the gist of what I was fiddling with. Maybe it'll be helpful.

Update mergeArray docs to show variadic union. Improve
mergeArray type for flow to allow variadic union, and add
a type test to verify.
@briancavalier briancavalier force-pushed the improve-ts-array-operation-types branch from a4128cd to c6220d5 Compare May 22, 2019 11:27
declare export function mergeArray <A> (ss: Array<Stream<A>>): Stream<A>

type MergeArray = <A>(Array<Stream<A>>) => Stream<A>
declare export function mergeArray <S> (ss: S): $Call<MergeArray, S>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unscriptable This isn't quite the same as the ZipArray case (which I still don't know how to do in Flow), but it's a useful technique nonetheless. It basically turns an array type into a union of the array's value types (removing the extra Stream nesting in this case).

@briancavalier
Copy link
Member Author

Figure out the implications for docs, given that Flow can't handle this (afaict)

After looking at the docs, I'm inclined just to go with them as is, and let the Flow limitations manifest themselves as type errors for now. IOW, let's see if the current Flow arity limitations are problem in reality before we try to figure out what to do about it.

@briancavalier briancavalier marked this pull request as ready for review May 22, 2019 11:39
@briancavalier briancavalier requested a review from Frikki May 22, 2019 11:53
@briancavalier briancavalier merged commit 9bdabda into master May 29, 2019
@briancavalier briancavalier deleted the improve-ts-array-operation-types branch May 29, 2019 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants