TypeScript 4.4 breaks logging exceptions from catch

[karl-run]

https://github.com/Microsoft/TypeScript-wiki/blob/main/Breaking-Changes.md#using-unknown-in-catch-variables

Users running with the --strict flag may see new errors around catch variables being unknown due to the new --useUnknownForCatchVariables flag, especially if the existing code assumes only Error values have been caught.

This causes issues with the typing in pino.error(exception) for example.

TS2769: No overload matches this call.
Overload 1 of 2, '(obj: object, msg?: string | undefined, ...args: any[]): void', gave the following error.
Argument of type 'unknown' is not assignable to parameter of type 'object'.
Overload 2 of 2, '(msg: string, ...args: any[]): void', gave the following error.<br/>Argument of type 'unknown' is not assignable to parameter of type 'string'.

Of course it works, because in this particular scenario the error is an actual thrown Error. But this change from TypeScript is because you can in theory throw anything. 🤷

[kibertoad]

@karl-run Can you elaborate a bit on where do you see the issue on Pino's side and how we could be handling it better? Converting from unknown generally should be done on a user side, I'm not sure we should be simply accepting any there. If such is user's desire, they could do catch (error: any)

[karl-run]

The reason this worked before is because I believe the error type TypeScript provides was any. Thus obj: object would happily receive any (well anything will happily receive an any).

Of course you could have the user handle this, but everyone having to do something like:

function makeSureIsError(error: unknown): error is Error {
    return error instanceof Error;
}

[...snip]
catch(error: unknown) {
  if (makeSureIsError(error)) {
      logger.error(error);
  }
}

Feels kind of tedious? Pino is already handling whatever we throw at it pretty nicely, and runtime nothing has changed, it's just the default type for error that has changed.

So what pino needs to do to support TS 4.4 "properly" is to extend the LogFn type to something along the lines of like this:

    interface LogFn {
        /* tslint:disable:no-unnecessary-generics */
        <T extends object>(obj: T, msg?: string, ...args: any[]): void;
        (obj: unknown, ...args: any[]): void;
        (msg: string, ...args: any[]): void;
    }

Because I assume doing

catch(e) {
  pinoLogger.error(e);
}

Isn't inherently a bad/not supported thing? Someone with more TypeScript expertise might be able to argue for or against using typing up unknown vs any. And I know as a library it is a bit more difficult to provide types that suits everyone's needs, but at least as an application developer avoiding any for the most part seems to be wise. Once you go any you don't actually have any typing anymore. 😄

[karl-run]

Worth noting that I was bumping some dependencies when I hit this error. Actually spent some time googling with no luck before it hit me that the type for error could have changed. I'm assuming others might end up down the same rabbit hole. So it would be nice for the consumers of pino that it's "a head of the curve", so to speak.

[kibertoad]

@climba03003 @pinojs/typescript Could use a second opinion here. Should we explicitly support unknown as a parameter?

[climba03003]

I would suggest doing the following.
If it fail the first rule because of unknown, the type will check against the second and allow unknown.

interface LogFn {
    /* tslint:disable:no-unnecessary-generics */
    <T extends object>(obj: T, msg?: string, ...args: any[]): void;
    (obj: unknown, msg?: string, ...args: any[]): void;
    (msg: string, ...args: any[]): void;
}
``

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.