Possible to elide more usages of Union types?

[jgrund]

Hi,

really nice work with the ts2fable update.

Looking through the generated node bindings, I am wondering if we can elide more usage of Union types.

I get that they fit the TS definition usage nicely, but they become un-ergonomic to use when working with bindings and having to look at which case maps to which variant.

Would it be possible to collapse into method overloads instead? If we do that, then Union types (or a DU wrapper in general) become unneeded (can pass in type directly) + still get the benefits of type-checking signature.

[ctaggart]

Yes, it would be great if we could do this in combination with #110 to have uniquely named overloads. Now accepting design proposals.

[MangelMaxime]

We need to be careful with methods overloads. F# have difficulties determining the right overload as stated here #110

[ctaggart]

One example is property setters. For this

    type [<AllowNullLiteral>] TlsOptions =
        abstract host: string option with get, set
        abstract port: float option with get, set
        abstract pfx: U2<string, ResizeArray<Buffer>> option with get, set
        abstract key: U4<string, ResizeArray<string>, Buffer, ResizeArray<obj>> option with get, set

It would be nice to be able to set easily for each case:

    type [<AllowNullLiteral>] TlsOptions =
        abstract host: string option with get, set
        abstract port: float option with get, set
        abstract pfx: U2<string, ResizeArray<Buffer>> option with get, set
        abstract pfxC1: string option with set
        abstract pfxC2: ResizeArray<Buffer> option with set
        abstract key: U4<string, ResizeArray<string>, Buffer, ResizeArray<obj>> option with get, set
        abstract keyC1: string option with set
        abstract keyC2: ResizeArray<string> option with set
        abstract keyC3: Buffer option with set
        abstract keyC4: ResizeArray<obj> option with set

[ctaggart]

For the unions in the functions, building off of a #110 proposal, we could add functions for each unique combination of cases.

        abstract readFileSync: path: U2<PathLike, float> * ?options: obj option -> Buffer
        /// Synchronously reads the entire contents of a file.
        abstract readFileSync: path: U2<PathLike, float> * options: U2<obj, string> -> string
        /// Synchronously reads the entire contents of a file.
        abstract readFileSync: path: U2<PathLike, float> * ?options: U2<obj, string> option -> U2<string, Buffer>

would become

        /// Synchronously reads the entire contents of a file.
        abstract readFileSync: path: U2<PathLike, float> * ?options: obj option -> Buffer
        abstract readFileSyncC1: path: PathLike * ?options: obj option -> Buffer
        abstract readFileSyncC2: path: float * ?options: obj option -> Buffer

        /// Synchronously reads the entire contents of a file.
        abstract readFileSyncP2: path: U2<PathLike, float> * options: U2<obj, string> -> string
        abstract readFileSyncP2C1C1: path: PathLike * options: obj -> string
        abstract readFileSyncP2C1C2: path: PathLike * options: string -> string
        abstract readFileSyncP2C2C1: path: float * options: obj -> string
        abstract readFileSyncP2C2C2: path: float * string -> string

        /// Synchronously reads the entire contents of a file.
        abstract readFileSyncP2N2: path: U2<PathLike, float> * ?options: U2<obj, string> option -> U2<string, Buffer>
        abstract readFileSyncP2N2C1C1: path: PathLike * ?options: obj option -> U2<string, Buffer>
        abstract readFileSyncP2N2C1C2: path: PathLike * ?options: string option -> U2<string, Buffer>
        abstract readFileSyncP2N2C2C1: path: float * ?options: obj option -> U2<string, Buffer>
        abstract readFileSyncP2N2C2C2: path: float * ?options: string option -> U2<string, Buffer>

Good/bad/ugly but useful?

[jgrund]

Hrrm, I haven't had many issues with method overloads.

I think the key to that is avoiding any usage of types that can carry multiple meanings i.e. obj and Function.

Node is heavily based on method (module function) overloads, and I think the most common usage of obj is usually an options object (which can be defined as a record).

[jgrund]

@ctaggart using the readFileSync method above, if we know the option rules, we can encode all possible variants without issue using overloads. Here is an example that will allow every combination (and know the return type) without the compiler complaining, and without any wrapper types:

example

We can do this by knowing the option contents, and because the node docs state: "If the encoding option is specified then this function returns a string. Otherwise it returns a buffer."

Of course, this approach requires an extra option record to encode optional fields as a separate type (and the ts bindings would have to follow a similar pattern as well), but the result would be a straight-forward experience when using the bindings.

[ctaggart]

Ah, so the main problem is that ts2fable is generating obj for TypeLiterals #93.

From node/index.d.ts, the 3 functions are:

    export function readFileSync(path: PathLike | number, options?: { encoding?: null; flag?: string; } | null): Buffer;
    export function readFileSync(path: PathLike | number, options: { encoding: string; flag?: string; } | string): string;
    export function readFileSync(path: PathLike | number, options?: { encoding?: string | null; flag?: string; } | string | null): string | Buffer;

With the TypeLiterals being:

{ encoding?: null; flag?: string; }
{ encoding: string; flag?: string; }
{ encoding?: string | null; flag?: string; }

The problem here is that is duck typing. We could potentially find and interfaces that implement the types and add the interface to them, but let's cross that bridge when needed. I'm starting to thing that these types should be extracted. In this case though, it is 3 different types.

[jgrund]

Yes, extracting the options from the bindings is definitely the direction to move in I think.

That said, there are cases (this being one), where the TS bindings need updating to be less ambiguous.

The third case:

export function readFileSync(path: PathLike | number, options?: { encoding?: string | null; flag?: string; } | string | null): string | Buffer;

Doesn't specify anything concrete in either options or result, which is why I excluded it in my earlier example.

We can know the return type based on the provided options for every scenario in this case, but the TS bindings don't account for this.

[alfonsogarciacaro]

I'm really enjoying this discussion. I'm very happy to see we're working together to make ts2fable a great tool! 🙌

On erased union types, just a reminder that we have the !^ operator (aka erased cast operator) that can make the profusion of erased union types in the bindings a bit more bearable. For example, for a setter:

    type [<AllowNullLiteral>] TlsOptions =
        abstract pfx: U2<string, IList<float>> option with get, set

    let opts: TlsOptions = jsOptions(fun o ->
        o.pfx <- !^"foo"
        o.pfx <- !^[|4., 5.|]
        // o.pfx <- !^6. // Doesn't compile, so we have type checking
    )

About using IList instead of ResizeArray to represent JS arrays, see #15 (comment)

But I know the operator is still not ideal and some people don't like it. I added a suggestion so F# implements erased unions natively to make the operator redundant, please vote and/or comment there if you want to see it happen: fsharp/fslang-suggestions#538

About using overloads in special places, or in general manually tweaking the bindings, I do agree this will probably still be helpful even if ts2fable reaches perfection (something that @ctaggart is bound to achieve! 😉) if only because the TS bindings are not always perfect as @jgrund says. I've been wondering if there's a good way to mark this manual changes so the tool won't touch that part (unless the corresponding TS part has changed, in which case a warning could be raised). Not sure if it'll be too complicated, in the last case we can just add a comment and use the git diffing to undo changes overwriting manual tweaks.

[jgrund]

I've been wondering if there's a good way to mark this manual changes so the tool won't touch that part (unless the corresponding TS part has changed, in which case a warning could be raised).

I've been thinking about this as well. It seems like we should prefer updating the DefinitelyTyped bindings when we find they can be improved. That way, all future ts2fable generations of a lib would not need further updating.

Barring that, I guess the simplest approach would be to generate git .patch files and see if they apply cleanly. If they don't then someone can come in and resolve the conflicts.

Finally, we should probably have tests for the updated bindings like DefinitelyTyped to ensure we have a stable (expected) interface post applying any patches.

[alfonsogarciacaro]

Definitely, being able to translate also the tests for the declarations and run them would be a huge win 👍