feat(bindNodeCallback): Works on overloaded functions

[kolodny]

I thought this PR would be a lot simpler 😅

Fixes #5942

[kolodny]

@benlesh Completely off topic, but shouldn't this include NaN?

[benlesh]

Yeah, probably.

[cartant]

NaN is not a type; it's a value, AFAICT; number is the type.

[kolodny]

Note, the only instance where this fails is if the overload callback has a different shape. For some reason changing Parameters< to OverloadedParameters< breaks the typings. Maybe @cartant knows why

[cartant]

Maybe @cartant knows why

TBH, my fantasy is that once v7 is released I'll no longer need deal with TypeScript and can ride off into the sunset and lead a simpler life.

[benlesh]

I don't understand this at all. haha. T extends a function that.. looks the same in 10 cases? But... isn't the same? But it's the same... but different.

[benlesh]

Wait... this is a type that is a "better Parameters<T>"? I still don't quite understand how it works.

[kolodny]

Yeah this is the magical overloaded function inference technique described at https://stackoverflow.com/questions/52760509/typescript-returntype-of-overloaded-function/52761156#52761156

[cartant]

Note, the only instance where this fails is if the overload callback has a different shape. For some reason changing Parameters< to OverloadedParameters< breaks the typings.

I have a few questions about this:

• What does the failure look like?
• And how hard will it be for a caller to figure out what's happening?
• How would a caller work around the problem?
• Are there many/any callback APIs that have differing callback shapes?

[kolodny]

What does the failure look like?

It falls through to the never case

And how hard will it be for a caller to figure out what's happening?

Anyone working with overloading functions should understand they're pretty busted from a TS inference POV

How would a caller work around the problem?

There really isn't any way to specify which overload they're interested in

Are there many/any callback APIs that have differing callback shapes?

I imagine this is pretty uncommon so I'm not too worried about it. The only place I see it being used is for encoding on fs.readFile which returns a string instead of a buffer when encoding is used

Note that the current PR is loads better then the default TS behavior of "let's pretend we only know about the last overload" approach

I can spend a bit more time on this but this problem is pretty in the weeds already

[kolodny]

Hmm, can't make much progress here. If anyone wants to try their hand at it, I isolated the issue to a playground:

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAKjgQwM5wGbo1CI4DkWBA3AFBkD0lcAEgKZT0pNwwAWzAJsBho-QB28CADdGAGwjIumAK6CAxjGARB2aHCYyAYsAn0AXBSwA6bVz0GAFEQgQCAGjjXGUZwCM5GAJRwAvAB8cADeZHARcF4Yph4AnjD0APJ8qPTw1HAAQt78UGQAvj7kZGYWVvS2GPZOoQXOrlDuUd5+QaHhkdGxCcmp6XCZOXyMhcUmqOb0uvqVdg7OIUKKEDyCAOaGhGiKwMAE9S5unq0BwWGRLTEwEACqYGCMAMJolX6ZqDBQwBtjJVQ0AAq7GA6FBbE4cFQIGQsDgYFwIFBvDi8iUKjUEOQ8FQ7AgcgksgA7tAANZwInADhwMSSaRceiyDAKZSqdRwCTAUnMMrTSyzYylFkYwTwxHIjBxAA8OlF9AAHolBFx0NZTOrYetUFtkII4gBtAC6bWCurigWsGEEW1lPi2ao1UC1WwAghIJDkYAAZNAwKVJcRQKQyRkABVhyBA6UYqBlgkCgRNcFD4rSUs6ER9nyl4agkejUFjsoTcAVSpVRyaOr1ziYqAJMC2PzycAASkmzXAAPxtuBbQT0QNkc4ZrTpORQUUO0ya1BJgdE5OpyrWOsQCTiWv0ABW9GUSYul0iK3U8EUHgCLC1pkgYGs4yPkSt09nDTc1biW-rEkbKD1B9HR8Il4SsoD8JgYAnUUmF3ZRGjA8ggKPNcN0qOsGwfICilHIpyAKCgT0+McZgMF1UDiJRLwRPAJTiS1JnKWZxkY0jyKUWxEk+Ah3hoJIAGkyBY+gyIoxQOPoLjFiKQZeIEoSRPYghOJgWoliUVYfk2bZUF2fZpMyfiKHktixKUiSVJ8UwOCEawL3aDwekSFIsHSHi4EM4zRPEyS6ks6zBFss4okcvoXJgNyPL5CoFNM5TVOWDSNi2Agdj2A4-M4AK7OCBz4ic-pwpkuAAEJSrgABJAA5J4klbVsAFEnkBLZgXBXF8UJKJmGQKEvk0krSooIaKBgOJHncwNgwZLgXSdOQo2EVAXWVVtx0nQExvoKVAWCfw4EBUtFSECtD0uF8nW1OBm0YOAXQARjtK7BBbVtEKPc7nSelsXQAJke66oDbN6zvVGcLqbZ6bpdABmf7IcB17AJcUHZwh76ABY4Ze4G4HwiIe31e64AAH1un6Sdu6GKZddHnFbQ1Ohaw7y3QU7Ig+y6Aduh60ZuxHHw53nAd+rG+ZxiJBa+qHYaFoGcM6AmidJ37qehumGYiJmy2O1mkclrn7tFhHxeRx1PoNv7Zf5yI8e7OBCbu6mfvVxn9uZnXTbB834dupMudbBX7ZdF3NbgAdAxKBVIDhUbxoDOkQy4XN80SQttt2iaE+m2b1nmoQYCWla1sEDbHnT-UAAZDXIKPoHgWPmHjoN6UZVbIPWzb08vJupsZHO88W5auDbqDS62nb9Tu6uKFrmPNtu91PSzP0Du15V0DNI0M9Xo717dntrB3ln7dB6wudoHxnDNQ07doPsw8HRg-H7R+oBr+Vo-r+fl+292983w0Gd9RmmcKDQEhp9SAn1AQAwGwOAEENNPIAA

[benlesh]

I think that TypeScript just can't solve this issue, TBH.

[kolodny]

I think that TypeScript just can't solve this issue, TBH.

In that case this PR should be good to go.

[cartant]

IDK why Last<OverloadedParameters<...>> has an undefined in it. How can a union of three tuples - FP - effect a union of four types? That makes little sense and that's what the callback parameters are extracted from, so that might be part of the problem:

declare function f(a: "a"): "a";
declare function f(a: "a", b: "b"): "ab";
declare function f(a: "a", b: "b", c: "c"): "abc";
type FP = OverloadedParameters<typeof f>; // = [a: "a"] | [a: "a", b: "b"] | [a: "a", b: "b", c: "c"]
type FR = OverloadedReturnType<typeof f>; // = "a" | "ab" | "abc"
type FABL = AllButLast<FP>; // = [] | [a: "a"] | [a: "a", b: "b"]
type FL = Last<FP>; // = "a" | "b" | "c" | undefined

I'm not sure this problem is worth the effort, TBH.

[kolodny]

Interesting, this definition of Last works as expected:

type Last<T> = T extends [...any, infer Last] ? Last : never;

[kolodny]

It's possible we're union-ing the arg types too soon, we may need them to flow as distinct tuples as long as possible. Honestly this is so technical that it's completely beyond what RxJS should be providing. If anyone wants to try their hand at it purely for the academic challenge. This is pushing the envelope a bit too far IMO.

Anyway I believe keeping the arg like this may be a solution

[cartant]

This is pushing the envelope a bit too far IMO.

I think one of the tests that we should apply to this library's types could be framed as a (rhetorical) question to the TypeScript maintainers: "did you expect these TypeScript features to be used like this?"

And if the answer is (likely to be) "no", we probably shouldn't do it and library users will just have to live with it and implement their own workaround or take the any escape hatch. TypeScript has become an increasing source of frustration for me, and I think that there are limits to which a language that is not statically typed can be tamed with an after-the-fact type system.

[cartant]

I've had another peek at this. I think the fundamental problem isn't extracting the parameters and the return types of the overloads, it's the disconnect between the all-but-first and last parameters in what's returned the call to bindCallback or bindNodeCallback, here:

rxjs/src/internal/observable/bindCallback.ts

Lines 17 to 20 in 2a501b2

	): (
	...arg: AllButLast<OverloadedParameters<Fn>>
	) => Observable<
	Last<Parameters<Fn>> extends () => any

I don't see how you can describe the relationship between them such that all-but-last and last correspond the the same overload signature. I don't think this is possible. And I think if we cannot make it work reliably with readFile, we should not do it.

[cartant]

I kinda hate myself for this, but a solution popped into my head whilst I was out for a drive. It involves using specific types for the bindCallback and bindNodeCallback results instead of the general-purpose OverloadedArgumentsAndReturnType.

If you look at the snippet, you'll see that the default and encoding overloads return the expected types. It would need more work done, as, IIRC, the bindCallback and bindNodeCallback implementations unpack arrays containing a single value and emit the value instead.

I still don't know how I feel about this solution, but I do know how I feel about TypeScript and I want it out of my head.

[cartant]

What we really want is a mapped type that can be applied to function types, but, as far as I'm aware, mapped types are only applied to object types (with keys) and to tuples. The (overload) signatures in function types have no keys so ... 🤷‍♂️

[cartant]

If we're going to refine the types for bindCallback and bindNodeCallback, I think we'll need to use the approach I mentioned in this comment. That is, I think we'll have to dispense with the more general OverloadedArgumentsAndReturnType and use types that are specific to bindCallback and bindNodeCallback.

What we really want is a TypeScript mechanism for mapping function signatures, but there is no such thing.

[cartant]

Blocking this 'cause it's not going to make the cut for v7, IMO.

[benlesh]

Core team meeting: Most people don't realize the function returned from bindCallback and bindNodeCallback is a "one-and-done" sort of thing. You can call it to get an observable, but that observable uses the same underlying AsyncSubject every time, and it's never reset/refreshed.

Maybe we need to deprecate this API and come up with something better?

[kolodny]

I kinda hate myself for this, but a solution popped into my head whilst I was out for a drive. It involves using specific types for the bindCallback and bindNodeCallback results instead of the general-purpose OverloadedArgumentsAndReturnType.

If you look at the snippet, you'll see that the default and encoding overloads return the expected types. It would need more work done, as, IIRC, the bindCallback and bindNodeCallback implementations unpack arrays containing a single value and emit the value instead.

I still don't know how I feel about this solution, but I do know how I feel about TypeScript and I want it out of my head.

So this has the downside that there's still no way to glue the args with the return type. I think I'll need to change the function into a fat arrow function in order to assign types as a single atom like this

Is this something we want to do? Also am I missing some magical way to keep it as a function (technically it'll be a breaking change unless we want to use this as a hack to force the type to be correct but still use an underlying function) while still having the args overload flavor agree with the return type?

[cartant]

Looking at the types for promisify, it's clear that the authors 'cheat' with the type declaration including:

    interface CustomPromisifyLegacy<TCustom extends Function> extends Function {
        __promisify__: TCustom;
    }

    interface CustomPromisifySymbol<TCustom extends Function> extends Function {
        [promisify.custom]: TCustom;
    }

    type CustomPromisify<TCustom extends Function> = CustomPromisifySymbol<TCustom> | CustomPromisifyLegacy<TCustom>;

    function promisify<TCustom extends Function>(fn: CustomPromisify<TCustom>): TCustom;

And readFile includes this, after the signatures:

    // NOTE: This namespace provides design-time support for util.promisify. Exported members do not exist at runtime.
    export namespace readFile {
        /**
         * Asynchronously reads the entire contents of a file.
         * @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
         * If a file descriptor is provided, the underlying file will _not_ be closed automatically.
         * @param options An object that may contain an optional flag.
         * If a flag is not provided, it defaults to `'r'`.
         */
        function __promisify__(path: PathLike | number, options?: { encoding?: null; flag?: string; } | null): Promise<Buffer>;

        // ... and many more

In light of this, I don't think a general solution is worth the effort. Well, not mine anyway. And I don't think we should attempt to leverage this trickery either.

Let's face it. There is no TypeScript feature for mapping signatures. And, also, bindCallback and it's friend bindNodeCallack are far from frequently-used. And the work around is simple: use an arrow function to match the signature you want to call.

I vote to close this. Emphatically.

[benlesh]