Narrow Callback Parameter Types

[imyourmanzi]

Request

A PR to update the type definitions to slightly narrow the number of possible types the stats callback parameters may be.

When a callback is being added to a stats function (usually inline), it's nice to have more specific types for those callback parameters.

Background

I'm a bit of a TypeScript fanatic so I recognize this is a really minor change and I have extra details here that may or may not be necessary!

After reviewing the types.d.ts for all usages of StatsCb and then verifying that those functions using StatsCbs all ended up using the _send method of the client, I've come to the conclusion that this is the most accurate type for StatsCb:

type StatsCb = (error?: Error | null, bytes?: number) => void;

However, this is very narrow and most user-written types that try to adhere to the current type would break (see Testing) due to the extra possibility that error could now be null.

My proposed change is:

type StatsCb = (error?: Error, bytes?: number) => void;

which makes the error parameter clearly optional (?) and also indicates that bytes is optional but that, when it is present, it is always a number.

This solution does break one scenario (see Testing, @ts-expect-error comment) in which the user has incorrectly typed their own callback function to assume that bytes is always present in the call, due to the fact that any dissuades TypeScript from further checking its identifier's types.

If any breaking change is too much work for this, I would then recommend (and subsequently update the PR) to use this type:

type StatsCb = (error?: Error, bytes?: number | any) => void;

It is the "least different" from the existing type but still offers a suggestion as to the type of the bytes parameter and is able to take advantage of optional parameters.

If you've made it this far, thank you!

Testing

I've created this TypeScript Playground with potential scenarios in which users may be utilizing the library based on its current types and then comparing that with the suggested change.

Playground Source (Backup)

/* potential existing user callback functions (typed) */

const cbA = (error: Error | undefined, bytes: any) => {}
const cbB = (error: Error | undefined, bytes: number) => {}
const cbC = (error: Error | undefined, bytes: unknown) => {}
const cbD = (error?: Error, bytes?: any) => {}
 // simulates no or very loose types
const cbE = (error: any, bytes: any) => {}
 // someone who figured out the correct types for their own project
const cbF = (error?: Error | null, bytes?: number) => {}
const cbG = (error: Error | undefined, bytes?: number) => {}
const cbH = (error?: unknown, bytes?: unknown) => {}


/* hot-shots package code currently */

// types
type StatsCbOriginal = (error: Error | undefined, bytes: any) => void;
type SomeStatsFnOriginal = (args: unknown, callback: StatsCbOriginal) => void;
// in practice, when TS merges types
const someStatsFnOriginal: SomeStatsFnOriginal = (args, callback)  => {}

// simulate user calling hot-shots stats function with their existing callbacks
someStatsFnOriginal(null,
  (
    error,
//  ^?
    bytes
//  ^?
  ) => {
})    
someStatsFnOriginal(null, cbA);
someStatsFnOriginal(null, cbB);
someStatsFnOriginal(null, cbC);
someStatsFnOriginal(null, cbD);
someStatsFnOriginal(null, cbE);
someStatsFnOriginal(null, cbF);
someStatsFnOriginal(null, cbG);
someStatsFnOriginal(null, cbH);


/* hot-shots package code, suggested */

// types
type StatsCbNew = (error?: Error, bytes?: number) => void;
type SomeStatsFnNew = (args: unknown, callback: StatsCbNew) => void;
// in practice, when TS merges types
const someStatsFnNew: SomeStatsFnNew = (args, callback)  => {}

// simulate user calling hot-shots stats function with their existing callbacks
someStatsFnNew(null,
  (
    error,
//  ^?
    bytes
//  ^?
  ) => {
})
someStatsFnNew(null, cbA);
// @ts-expect-error: fails because the user has typed their callback to always expect `bytes` because `bytes: any` originally allowed for that
someStatsFnNew(null, cbB);
someStatsFnNew(null, cbC);
someStatsFnNew(null, cbD);
someStatsFnNew(null, cbE);
someStatsFnNew(null, cbF);
someStatsFnNew(null, cbG);
someStatsFnNew(null, cbH);

[bdeitte]

@imyourmanzi Apologies on the delay on this, and that is some good learning for me in here. I'm fine with it as-is, I will just make sure it doesn't go in a patch update. Thanks.

[imyourmanzi]

@bdeitte No worries! Glad to see it was a helpful PR, let me know if there are any concerns that arise