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

TS Typing issue with bindNodeCallback and readdir #5942

Closed
AlpBilgin opened this issue Dec 24, 2020 · 6 comments
Closed

TS Typing issue with bindNodeCallback and readdir #5942

AlpBilgin opened this issue Dec 24, 2020 · 6 comments

Comments

@AlpBilgin
Copy link

AlpBilgin commented Dec 24, 2020

Bug Report

Current Behavior
Type of Observable created by bindNodeCallback(readdir) is not properly advertised.
causes VSCode to issue following complaint:
Type 'Observable<unknown>' is not assignable to type 'Observable<string[]>'.

Reproduction
Paste following code into a VSCode editor.

import {readdir} from "fs";
import {Observable,bindNodeCallback} from "rxjs";

function readFileObs(pathname: string): Observable<string[]> {
  const readFileAsObservable = bindNodeCallback(readdir);
  return readFileAsObservable(pathname);
}

Expected behavior
bindNodeCallback(readdir) should advertise Observable<string[]> as return type.

Environment

  • Runtime: node: v15.2.1
  • RxJS version: 6.6.3
  • (If bug is related) Loader, build configuration: TS: 4.1.3, VSCode: 1.52.1 (MacOS)

Additional context/Screenshots
node typings correctly advertises readdir, but bindNodeCallback somehow fails to advertise the correct type.
image

@AlpBilgin
Copy link
Author

also I already told the type checker that its okay:

return readFileAsObservable(pathname) as Observable<String[]>;

@benlesh
Copy link
Member

benlesh commented Jan 3, 2021

It appears this works better in 7.0, but not perfect. For 6.0, I recommend just casting as Observable<Dirent[]> as a workaround.

@benlesh
Copy link
Member

benlesh commented Jan 3, 2021

Note to @cartant and @kolodny (who worked on this for v7), looking more at this, there's simply no way to perfectly type bindNodeCallback and bindCallback with functions like readdir, because it has variadic arguments. In other words it's types are like:

    export function readdir(
        path: PathLike,
        options: { encoding: BufferEncoding | null; withFileTypes?: false } | BufferEncoding | undefined | null,
        callback: (err: NodeJS.ErrnoException | null, files: string[]) => void,
    ): void;

    export function readdir(path: PathLike, options: { encoding: "buffer"; withFileTypes?: false } | "buffer", callback: (err: NodeJS.ErrnoException | null, files: Buffer[]) => void): void;


    export function readdir(
        path: PathLike,
        options: BaseEncodingOptions & { withFileTypes?: false } | BufferEncoding | undefined | null,
        callback: (err: NodeJS.ErrnoException | null, files: string[] | Buffer[]) => void,
    ): void;

    export function readdir(path: PathLike, callback: (err: NodeJS.ErrnoException | null, files: string[]) => void): void;

    export function readdir(path: PathLike, options: BaseEncodingOptions & { withFileTypes: true }, callback: (err: NodeJS.ErrnoException | null, files: Dirent[]) => void): void;

I've messed with it a lot, and I can't really get the types to infer properly for these given how they work, and I'm not sure TypeScript could ever support it.

In fact, while trying to properly type this, I tried to see what TypeScript does with Function.prototype.bind and I discovered that it has issues of its own when dealing with functions that have variadic arguments: microsoft/TypeScript#42196

@kolodny
Copy link
Member

kolodny commented Jan 4, 2021

This is a TS limitation with the way TS only does inference on the last overload of a function. Note that if you used the last overload in this case in rxjs v7 this will work as expected: https://codesandbox.io/s/ecstatic-sara-r2uxp?file=/src/index.ts

image

Also note, there's no way to tell TS which overload you want to use (like readdir[2] or something similar)

Short of converting the overload to a conditional type (please don't), there isn't a way to get this to work on the RxJS side of things 😢

@kolodny
Copy link
Member

kolodny commented Jan 13, 2021

It appears there is a way to get this to work based on what I stumbled upon today:

Expand to see the full code example
import * as fs from 'fs';

type OverloadedArgumentsAndReturnType<T> =
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
        (...args: infer A5): infer R;
        (...args: infer A6): infer R;
        (...args: infer A7): infer R;
        (...args: infer A8): infer R;
        (...args: infer A9): infer R;
        (...args: infer A10): infer R;
    } ? [A1|A2|A3|A4|A5|A6|A7|A8|A9|A10, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
        (...args: infer A5): infer R;
        (...args: infer A6): infer R;
        (...args: infer A7): infer R;
        (...args: infer A8): infer R;
        (...args: infer A9): infer R;
    } ? [A1|A2|A3|A4|A5|A6|A7|A8|A9, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
        (...args: infer A5): infer R;
        (...args: infer A6): infer R;
        (...args: infer A7): infer R;
        (...args: infer A8): infer R;
    } ? [A1|A2|A3|A4|A5|A6|A7|A8, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
        (...args: infer A5): infer R;
        (...args: infer A6): infer R;
        (...args: infer A7): infer R;
    } ? [A1|A2|A3|A4|A5|A6|A7, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
        (...args: infer A5): infer R;
        (...args: infer A6): infer R;
    } ? [A1|A2|A3|A4|A5|A6, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
        (...args: infer A5): infer R;
    } ? [A1|A2|A3|A4|A5, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
        (...args: infer A4): infer R;
    } ? [A1|A2|A3|A4, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
        (...args: infer A3): infer R;
    } ? [A1|A2|A3, R] :
    T extends {
        (...args: infer A1): infer R;
        (...args: infer A2): infer R;
    } ? [A1|A2, R]  :
    T extends (...args: infer A) => infer R ? [A, R] : never;

type OverloadedArguments<T> = OverloadedArgumentsAndReturnType<T>[0];
type OverloadedReturnType<T> = OverloadedArgumentsAndReturnType<T>[1];


let foo: OverloadedArgumentsAndReturnType<typeof fs.readFile>[0];

fs.readFile('test', {}, () => {})

type Head<T extends readonly unknown[]> = T[0]
type Rest<T extends readonly unknown[]> = T extends T ? ((...args: T) => any) extends (a: any, ...rest: infer R) => any ? R : never : never
type AllButLast<T extends any[]> = T extends T ? T extends [ ...infer Head, any ] ? Head : never;


let foo2: AllButLast<OverloadedArguments<typeof fs.readFile>> = {} as never;
foo2 = [''] // ok
foo2 = ['', {encoding: 'ascii'}] // ok
foo2 = ['', {encoding: 'asci'}] // error

Note the last bit where we can see the overload parameter types working:

let foo2: AllButLast<OverloadedArguments<typeof fs.readFile>> = {} as never;
foo2 = [''] // ok
foo2 = ['', {encoding: 'ascii'}] // ok
foo2 = ['', {encoding: 'asci'}] // error

I need to spend a bit of time to incorporate this into RxJS but I don't see why we won't just use this more powerful inference for functions.

@kolodny kolodny added the AGENDA ITEM Flagged for discussion at core team meetings label Jan 13, 2021
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Feb 10, 2021
@cartant
Copy link
Collaborator

cartant commented May 23, 2021

We are not going to attempt to make any changes to the types to that the override signatures are 'mapped' to the observable type for the reasons given in this comment.

The workaround it to pass an error function to bindNodeCallback where the arrow function declares parameters matching the signature that you wish to call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants