adaltas / node-http-status

Utility to interact with HTTP status code in Node.js
Other
441 stars 55 forks source link

Unable to properly infer numeric keys in TypeScript type with mixed key types #48

Closed aronmal closed 6 months ago

aronmal commented 6 months ago

Describe the bug

When utilizing the http-status package, there is a bug related to inferring numeric keys in TypeScript when working with a mixed type declaration within the HttpStatus interface in index.d.ts.

To Reproduce

To reproduce the issue, consider the following TypeScript code snippet:

import httpStatus from "http-status"; // object itself
import type { HttpStatus } from "http-status"; // interface

// example for comparison
const testStatus = {
  100: "",
  "100_NAME": "",
  "100_MESSAGE": "",
  CONTINUE: 100,
  101: "",
  "101_NAME": "",
  "101_MESSAGE": "",
  SWITCHING_PROTOCOLS: 101,
  102: "",
  "102_NAME": "",
  "102_MESSAGE": "",
  PROCESSING: 102,
  103: "",
  "103_NAME": "",
  "103_MESSAGE": "",
  EARLY_HINTS: 103,
};

// Define a custom type to extract numeric keys
type NumberKeys<T> = {
  [K in keyof T]: T[K] extends number ? (string extends K ? never : K) : never;
}[keyof T];

// Define a function with a parameter that should only accept numeric keys
export function Response(code: NumberKeys<HttpStatus>) {
  const thisCode = httpStatus[code];
  // Rest of your function logic
}

In this case, the types do not work correctly and autocompletion does show any possible string in the ide image

When changing the type input to the example object:

- export function Response(code: NumberKeys<HttpStatus>) {
+ export function Response(code: NumberKeys<typeof test>) {
    const thisCode = httpStatus[code];
    // Rest of your function logic
  }

it then works and shows the correct types

image

Additionally, the unnecessary declaration readonly [key: number]: string | undefined; in the HttpStatus interface contributes to the issue and might be impacting TypeScript type inference.

Additional context

The issue seems to be associated with the mixed type declaration within the HttpStatus interface in index.d.ts, specifically:

export = httpStatus;

declare const httpStatus: httpStatus.HttpStatus;

declare namespace httpStatus {
  interface HttpStatus {
    // not important for this issue, but I still see no sense in this either
    readonly [key: number]: string | undefined;

    // this causes the issue
    readonly [key: string]:
    | string
    | number
    | HttpStatusClasses
    | HttpStatusExtra
    | undefined;

This declaration allows for both string and number keys, leading to unexpected behavior in TypeScript type inference. The bug impacts TypeScript type inference in projects utilizing the http-status package, particularly when trying to enforce strict typing for numeric keys. The unnecessary readonly [key: number] declaration are causing the issue and should be considered for removal.

aronmal commented 6 months ago

Is there a reason for the readonly [key: number]? If not I can create a pull request, but I wanted to ask first if it has an origin.

wdavidw commented 6 months ago

I don't think so, or I don't have a memory for it. Please propose your change if you feel confident. I personally don't use TS.

wdavidw commented 6 months ago

Also, add a unit test to support your claim.

aronmal commented 6 months ago

Also, add a unit test to support your claim.

I've addressed the comment about adding a unit test. While there isn't a specific test to showcase the issue due to the nature of the problem (lack of strictness in types allowing more keys than intended), I did enhance the existing unit tests because during this process, a type error in the test file surfaced. But it has been taken care of.

If you have any further suggestions, let me know!