kogosoftwarellc / open-api

A Monorepo of various packages to power OpenAPI in node
MIT License
895 stars 237 forks source link

Property 'default' of type 'Response | undefined' is not assignable to string index type 'Response' #742

Closed mirceanis closed 3 years ago

mirceanis commented 3 years ago

using package open-api in a project with "strict": true in tsconfig.json causes the error:

Property 'default' of type 'Response | undefined' is not assignable to string index type 'Response'

It would seem that explicitly accepting undefined in ResponsesObject fixes it:

  export interface ResponsesObject {
    [index: string]: Response | undefined;
    default?: Response;
  }

This happens since version 9.0.0

dnalborczyk commented 3 years ago

encountered the same issue.

the interface defines that all indexers have the same signature, with default? also returning undefined, it deviates from the indexer definition.

https://stackoverflow.com/questions/45258216/property-is-not-assignable-to-string-index-in-interface

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBASwHY2FAZgQwMbDgJWAGdIkjiB5AIwCtht4BvAWACg4O4BtZAE1ABccIjCjIA5gF0hhEhDLAA3G05x+WAK4AbGAH4ZxUuWXs4AXyA

not sure if that fits the intend either, unless all other properties can return Response or (be) undefined.

export interface ResponsesObject {
  [index: string]: Response | undefined;
  default?: Response;
}

change commit: https://github.com/kogosoftwarellc/open-api/commit/dae0e37fb6e1b67265111fca3ef7f2049736cfb8

mirceanis commented 3 years ago

I don't know about the intents here. I proposed this fix purely from a typescript strict compilation perspective. I suppose it would also work if default is always expected:

export interface ResponsesObject {
  [index: string]: Response;
  default: Response;
}

Later edit: I'm not proposing this as a solution since it probably does not reflect reality. I'm only saying that this syntax also satisfies the compiler.

Kosta-Github commented 3 years ago

default is definitely optional, therefore default?: Response is required.

IMHO, [index: string]: Response | undefined is reasonably, since you will not get a Response object back for a random string, e.g., resp['bla'] or resp.hello ... therefore, you should be "forced" to check for undefined...

henhal commented 3 years ago

Record types where the key is all strings is bound to cause problems: Record<string, X> or {[key: string]: X} both mean that you may index the type with any string and expect to get an X back.

So in this case, the type specifies that every value of type ResponsesObject has the properties foo, bar and lolcat, and all other infinite combinations of characters that form strings ...

For types where the key range is not a known union or similar, you'd better use | undefined on the values. Also, if doing that, in this case, adding the default property at all is redundant.

This should do?

interface ResponsesObject {
  [index: string]: Response | undefined;
}

What @mirceanis suggests would mean that response objects always have a default response, and also have responses for any possible string you could possibly try to index it with.

(BTW: temp workaround to get on for me was to use skipLibCheck: true in tsconfig.json.)

mirceanis commented 3 years ago

Please ignore my second "solution". It only satisfies the typescript compiler, but doesn't reflect reality in any way.

The actual solution I'm proposing is in the description of this issue and also in PR #743

Thank you for the skipLibCheck workaround, @henhal. I'll try that.

henhal commented 3 years ago

Thanks @mirceanis, I hadn't seen the PR. Great work.

Morl99 commented 3 years ago

@jsdevel I don't mean to be pushy, but if there is any chance that you could take a look at the linked PR, it would be really helpful. For the moment, we are blocked to update to 9.0.2 because of this.

Thank you for your hard work!