ioredis icon indicating copy to clipboard operation
ioredis copied to clipboard

Loosen callback typings

Open luin opened this issue 3 years ago • 6 comments
trafficstars

In @types/ioredis, callbacks are defined as (err: Error | null, res: T) => void, whereas in ioredis v5, they are (err?: Error | null, res?: T) => void.

The intention was to indicate the res can be absent when there is an error. However, given TypeScript doesn't provide a way to achieve "if there is no error, then res should be fulfilled", developers have to check res in every callback even if they have already checked err:

return new Promise((resolve, reject) => {
  redis.get('foo', (err, res) => {
    if (err) reject(err);
    // Can't use `else` here
    if (typeof res !== 'undefined') resolve(res);
  });
});

I feel it's a bit inconvenient, and apparently, people have different opinions on it: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/43583.

I lean towards keeping in line with the Node.js way: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/884d68c/types/node/fs.d.ts#L2416 though no strong opinion.

Open to any suggestions! 😄

luin avatar Mar 27 '22 01:03 luin

Why not model the type exactly after ist usage?

export type Callback<T = any> = (err: Error, result: never) => void | (err: undefined, result: T) => void;

By the way, I believe that the more "TypeScript-y" default type for T would be unknown, resulting in

export type Callback<T = unknown> = (err: Error, result: never) => void | (err: undefined, result: T) => void;

fabrykowski avatar Mar 30 '22 09:03 fabrykowski

@fabrykowski Don't think that works. TypeScript asks to give the parameters types.

luin avatar Mar 30 '22 09:03 luin

My bad – I didn't check it with a compiler 😬

You can use tuples in rest parameters to achieve the goal:

export type Callback<T = unknown> = (...args: [err: Error, result: never] | [err: undefined, result: T]) => void;

playgroud

fabrykowski avatar Mar 30 '22 09:03 fabrykowski

In that case seems result will never be never 😢

luin avatar Mar 30 '22 09:03 luin

If we check for the existence of the error, then result does become never. Unfortunately if we don't check, the type of result is never | T, which TypeScript simplifies to T. Of course result can be set to undefined:

export type Callback<T = unknown> = (...args: [err: Error, result: undefined] | [err: undefined, result: T]) => void;

playground We lose some of the semantics, but gain a functioning definition 😅

fabrykowski avatar Mar 30 '22 09:03 fabrykowski

@fabrykowski Oh that's awesome! I didn't know that was possible. Looks like it's only supported from TypeScript 4.6 but seems to be a good progressive enhancement.

luin avatar Mar 31 '22 00:03 luin