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

Use a conditional type to represent the return value of map #403

Merged
merged 2 commits into from
Dec 9, 2019

Commits on Dec 4, 2019

  1. Use a conditional type to represent the return value of map

    Although fluture-js#401 solved the case for using `map` on a `ConcurrentFutureInstance`, it broke `FutureInstance#pipe(map(...))`:
    
    ```typescript
    import { ConcurrentFutureInstance, FutureInstance, map } from 'fluture';
    
    declare const x: FutureInstance<Error, number>;
    declare const y: ConcurrentFutureInstance<Error, number>;
    declare const double: (x: number) => number;
    
    const v1 = map(double)(x); // ok; FutureInstance<Error, number>
    const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
    const v2 = x.pipe(map(double)) // error
    ```
    
    This could potentially be down to TypeScript selecting the wrong overload (maybe because `ConcurrentFutureInstance` is the more restrictive type?)
    
    So, I came up with this version instead, which uses a Conditional type to represent the mapper function instead. It's not ideal from an ergonomics perspective, as you end up with the whole conditional type appearing in Intellisense information. I've never been fond of that. But, it does fix (and preserve all type information) for the three forms above:
    
    ```typescript
    import { ConcurrentFutureInstance, FutureInstance, /*map*/ } from 'fluture';
    
    declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
        T extends FutureInstance<infer U, infer V> ?
        FutureInstance<U, V> :
        T extends ConcurrentFutureInstance<infer U, infer V> ?
        ConcurrentFutureInstance<U, V> :
        never;
    
    declare const x: FutureInstance<Error, number>;
    declare const y: ConcurrentFutureInstance<Error, number>;
    declare const double: (x: number) => number;
    
    const v1 = map(double)(x); // ok; FutureInstance<Error, number>
    const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
    const v2 = x.pipe(map(double)) // ok; FutureInstance<Error, number>
    ```
    
    I did also experiment with simply swapping the order of the overloads around:
    
    ```typescript
    import { ConcurrentFutureInstance, FutureInstance, /*map*/ } from 'fluture';
    
    declare function map<RA, RB>(mapper: (value: RA) => RB): {
        <L>(source: ConcurrentFutureInstance<L, RA>): ConcurrentFutureInstance<L, RB>;
        <L>(source: FutureInstance<L, RA>): FutureInstance<L, RB>;
    }
    
    declare const x: FutureInstance<Error, number>;
    declare const y: ConcurrentFutureInstance<Error, number>;
    declare const double: (x: number) => number;
    
    const v1 = map(double)(x); // ok; FutureInstance<Error, number>
    const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
    const v2 = x.pipe(map(double)) // almost ok; FutureInstance<unknown, number>
    ```
    
    Observe that the left type of `v2` is `unknown` when it should be `Error`. I suspect this is down to that hanging `<L>` type parameter, and the only way to eliminate that (afaik) is to use a conditional type instead. Which, coincidentally, is where we came in.
    wms committed Dec 4, 2019
    Configuration menu
    Copy the full SHA
    c92dcd0 View commit details
    Browse the repository at this point in the history

Commits on Dec 5, 2019

  1. Tighten up the type definition of map some more

    1. The previous commit did not solve for the case where the mapper function changes the type of the right value:
    
    ```typescript
    declare const toString: (x: number) => string;
    
    const v3 = map(toString)(x); // ok but incorrect; FutureInstance<Error, number>
    const v3p = map(toString)(y); // ok but incorrect; ConcurrentFutureInstance<Error, number>
    const v4 = x.pipe(map(toString)) // ok but incorrect; FutureInstance<Error, number>
    ```
    
    This is addressed by re-using the `RA` and `RB` type parameters instead of inferring:
    
    ```typescript
    declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
        T extends FutureInstance<infer L, RA> ?
        FutureInstance<L, RB> :
        T extends ConcurrentFutureInstance<infer L, RA> ?
        ConcurrentFutureInstance<L, RB> :
        never;
    
    const v3 = map(toString)(x); // ok; FutureInstance<Error, string>
    const v3p = map(toString)(y); // ok; ConcurrentFutureInstance<Error, string>
    const v4 = x.pipe(map(toString)) // ok; FutureInstance<Error, string>
    ```
    
    2. However, this introduces a potentially hard to track down error when using the wrong type:
    
    ```typescript
    const v5 = map(toString)(z); // never w/ no error at callsite; sub-optimal
    const v5p = map(toString)(z); // never w/ no error at callsite; sub-optimal
    const v6 = z.pipe(map(toString)) // never w/ no error at callsite; sub-optimal
    ```
    
    To address this, we place a constraint on `T`:
    
    ```typescript
    declare function map<RA, RB>(mapper: (value: RA) => RB): <T extends FutureInstance<any, RA> | ConcurrentFutureInstance<any, RA>>(source: T) =>
        T extends FutureInstance<infer L, RA> ?
        FutureInstance<L, RB> :
        T extends ConcurrentFutureInstance<infer L, RA> ?
        ConcurrentFutureInstance<L, RB> :
        never;
    
    const v1 = map(double)(x); // ok; FutureInstance<Error, number>
    const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
    const v2 = x.pipe(map(double)) // ok; FutureInstance<Error, number>
    
    const v3 = map(toString)(x); // ok; FutureInstance<Error, string>
    const v3p = map(toString)(y); // ok; ConcurrentFutureInstance<Error, string>
    const v4 = x.pipe(map(toString)) // ok; FutureInstance<Error, string>
    
    const v5 = map(toString)(z); // error at callsite; that's good
    const v5p = map(toString)(z); // error at callsite; that's good
    const v6 = z.pipe(map(toString)) // error at callsite; that's good
    ```
    wms committed Dec 5, 2019
    Configuration menu
    Copy the full SHA
    5f5a86c View commit details
    Browse the repository at this point in the history