-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Add branch awareness to TypeScript types (auto-expanding) #435
base: master
Are you sure you want to change the base?
Conversation
The type for 'alt' did not allow users to supply a future with a predetermined branch (when one of the branches is never). This commit adds overloads for alt that allow TypeScript to understand how our code recovers when there is no concrete type in either of the branches of the first Future. This makes the type inference much better, but it also means that mixed types are no longer considered a type error. Finally, this commit adds tests for the TypeScript type of alt.
The type for 'and' did not allow users to supply a future with a predetermined branch (when one of the branches is never). This commit adds overloads for 'and' that allow TypeScript to understand how our code recovers when there is no concrete type in either of the branches of the first Future. This makes the type inference much better, but it also means that mixed types are no longer considered a type error. Finally, this commit adds tests for the TypeScript type of 'and'.
Makes the typescript types for ap branch aware and adds type tests. This improves type inference, but makes it allowed to combine two Futures with different rejection types.
Makes the typescript types for bichain branch aware and adds type tests. This improves type inference, but makes it allowed to combine two Futures with different rejection or resolution types.
This comment has been minimized.
This comment has been minimized.
Seems nice! I find fp-ts TaskEither to work like magic, never had to add a single type EVER. Maybe @gcanti has the clue for this issues. |
@codingedgar maybe that's because with import * as E from 'fp-ts/lib/Either'
import { pipe } from 'fp-ts/lib/function'
const x1 = E.chain(() => E.right(42))(E.left('x')) // <= error
const x2 = pipe( // <= ok
E.left('x'),
E.chain(() => E.right(42))
)
export declare function left<E = never, A = never>(e: E): Either<E, A>
export declare function right<E = never, A = never>(a: A): Either<E, A> |
Ahh, so in fp-ts you consciously work around the " The same is currently true for Fluture (with the exception of the import * as fl from 'fluture'
const x1 = fl.bichain(() => fl.resolve(42))(fl.resolve)(fl.reject('x')) // <= error
const x2 = fl.reject('x').pipe( // <= ok
fl.bichain(() => fl.resolve(42))(fl.resolve)
) I am curious about your opinion now, @gcanti. Do you think it's worth the trade-off? Essentially making it so: E.chain(() => E.right(42))(E.left('x')) // <= no longer an error :)
E.chain(() => E.left(42))(E.left('x')) // <= no longer an error too :( |
Not always if I'm not mistaken, take import * as fl from 'fluture'
const x = fl.reject('a').pipe(fl.alt(fl.resolve(42))) // error that's why I'm using defaults for the constructors type parameters const reject: <L = never, R = never>(reason: L) => fl.FutureInstance<L, R> = fl.reject
const resolve: <L = never, R = never>(value: R) => fl.FutureInstance<L, R> = fl.resolve
const y = reject('a').pipe(fl.alt(resolve(42))) // ok
Well, it really depends on the DX you want to provide, some users may be happy with an auto-widening API. <E, A, B>(f: (a: A) => E.Either<E, B>) => (ma: E.Either<E, A>) => E.Either<E, B> and a <D, A, B>(f: (a: A) => E.Either<D, B>) => <E>(ma: E.Either<E, A>) => E.Either<D | E, B> |
Oh, good eye. I hadn't realized that! I don't use TypeScript often enough to notice these things, I must admit. But I intend to do a bit more with Deno, so I might. :)
I don't understand how those defaults are making a difference. o.0 const resolve: <R>(value: R) => fl.FutureInstance<never, R> = fl.resolve
const resolve_: <L = never, R = never>(value: R) => fl.FutureInstance<L, R> = fl.resolve
const x = fl.reject('a').pipe(fl.alt(resolve(42))) // not ok
const y = fl.reject('a').pipe(fl.alt(resolve_(42))) // ok
const a = resolve(42); // FutureInstance<never, number>
const b = resolve_(42); // FutureInstance<never, number>
const x = fl.reject('a').pipe(fl.alt(a)) // not ok
const y = fl.reject('a').pipe(fl.alt(b)) // not ok I must be misunderstanding something about how TypeScript assigns generics. Is it that in the
Oh, I can see that making a lot of sense for a TypeScript-first library! That gives an interesting perspective. I'm going to let that sink in. :) |
const resolve: <R>(value: R) => fl.FutureInstance<never, R> = fl.resolve
const b = resolve_(42); // FutureInstance<never, number> When you do the assignment on Try this: const x = fl.reject('a').pipe(fl.alt(resolve(42))) // not ok
const y = fl.reject('a').pipe(fl.alt(resolve_(42))) // ok or pipe(fl.reject('a'), fl.alt(resolve_(42))) |
I created a strict (non-auto-expanding) variant of this pull request, but it introduces new challenges: #438 |
In the past, I've always tried to use TypeScript to force type consistency onto users of the library: #374 has a good summary of this history.
To explain my motivation for this change, let's first introduce the idea of certain and uncertain Futures. I'll refer to a Future where one of the two branches is
never
as a "certain Future". For example,resolve (42)
is a certain Future of anumber
, because the rejection type isnever
.In #374, I looked for a way to allow TypeScript to infer types when chaining a certain Future. This is quite a common scenario, and one that leaves a user with a consistent type. For example
chain (x => resolve (42)) (reject ('x'))
is allowed, becauseFuture String a
is assignable toFuture b Number
. In TypeScript, which does not have type variables like that, the two Futures are typed asFuture<String, never>
andFuture<never, Number>
. These are not assignable both ways, in TypeScript. To fix this problem, I decided to relax the types ofchain
, andchainRej
functions, so that the resulting Future would beFuture<String | never, never | Number>
, which works becauseT | never
isT
. The downside is that forced type consistency was lost for these functions:chain (x => reject (42)) (reject ('x'))
would come out asFuture<number | string, never>
without it being considered a type error.A few weeks ago, I realized that this same problem plagues many more functions in the Fluture library. It's not just
chain
andchainRej
, but alsoalt
,and
,ap
, or any other function that combines two or more Futures.This PR will relax the types of all of those combinators, hugely improving type inference when using typed Fluture, at the cost of forced type consistency across the library. I managed to find a middle-ground though, where type consistency is lost only when a Future is uncertain. I achieved this by essentially encoding the branching rules for each combinator into its TypeScript type. So whenever a Future is certain, TypeScript can figure out which branch will end up where, and it won't have to merge the types.
I have many more types to go over, but I'm opening this as a draft because from here on out, each new commit will just repeat the same pattern, so it's a good time to gather feedback.
I am quite excited about this change. When this is released, TypeScript users will find a massive improvement to the convenience of working with Fluture in TypeScript. Inference will take away most of the manual typing work which was needed, and the new branch awareness will seem like magic: "How does TypeScript know that this long chain of Futures will end up in a resolved state?!"
I think the middle ground I've found gives users the best of two worlds. Incorrect use of the API under uncertain branching conditions will be expressed as a mixed type, rather than a TypeError. Users will know they need to deal with the mixed type, either by using type guards at a later stage, or changing the way to use Fluture's API. But when branching conditions are certain, types can remain consistent.
/cc @codingedgar, @tetsuo, @gcanti