Type for done not inferred

[felixfbecker]

The way how it is defined as a callable ITestDefinition interface makes it impossible for TS to infer the type of the done callback.

 declare var it: Mocha.ITestDefinition;
 interface ITestDefinition {
        (expectation: string, assertion?: () => void): ITest;
        (expectation: string, assertion?: (done: MochaDone) => void): ITest;
        only(expectation: string, assertion?: () => void): ITest;
        only(expectation: string, assertion?: (done: MochaDone) => void): ITest;
        skip(expectation: string, assertion?: () => void): void;
        skip(expectation: string, assertion?: (done: MochaDone) => void): void;
        timeout(ms: number): void;
    }

Doing

it('whatever', done => {

});

yields

src/test/dbgp.ts(24,53): error TS7006: Parameter 'done' implicitly has an 'any' type.

This used to work. Now we have to do

it('whatever', (done: MochaDone) => {

});

[blakeembrey]

Yeah, that's really bad. It sucks that TypeScript can't handle this because it's a really legitimate use-case. I've run into it on a few other places now (Express.js, Passport.js, etc). Even making the argument optional messes up things more using strictNullChecks, I had to do a bunch of done && done() recently using jest. What do you think is the best fix?

[felixfbecker]

In the Express typings I worked around this by not using a callable interface, but duplicating the definitions and using method overloads instead of using the interface. I don't know though if that would work here - it is a global variable, so I don't think we can "overload" it. But why did it work before? Were there changes?

[felixfbecker]

Ah I think it never worked, but I just recently activated noImplicitAny in the only project where I use the done callback. In all other projects I exclusively used promises and async functions.

[blakeembrey]

@felixfbecker FWIW, I've never had the Express.js typings work out of the box. Not sure if we're doing something different with them.

[felixfbecker]

What do you mean with you never had the express typings work out of the box?

[blakeembrey]

I've never been able to do app.use(function (req, res, next) {}) without typing app.use(function (req: express.Request, res: express.Response, next: express.NextFunction) {}) explicitly.

[felixfbecker]

Ah, now I know what youre talking about. It worked when the definition was only

 get(path: PathArgument, ...handlers: (RequestHandler | RequestHandler[])[]): this

but then someone reported that ErrorHandler must be allowed too, and would have changed it into:

 get(path: PathArgument, ...handlers: (RequestHandler | ErrorHandler | (RequestHandler | ErrorHandler)[])[]): this

so I sacrificed the inference in types/express#2
actually as I now look at this, I remember the primary motivation was to allow an indefinitely nested array of handlers in a handler type that recursively references itself, which would not be possible with inline types. But the current definitions are not build that way and only allow 2 levels... Maybe TS threw an error when I tried to do the recursive thing.

[felixfbecker]

What to take from this is: Inference does not work when the type is a type alias/interface.

[unional]

Should there be an issue open on TS side?

[felixfbecker]

Probably

[unional]

However, if it works as you wanted it to, will it still have the The type 'MochaDone' is used but cannot be named problem?

(if I understand correctly on what you want) 🌷