openapi-typescript icon indicating copy to clipboard operation
openapi-typescript copied to clipboard

Query param type safety only works for the first param.

Open Noahkoole opened this issue 1 year ago • 9 comments

Description

When supplying multiple query params. it seems that when entering the first one correct it turns in to some sort of "Any" type. Allowing for any type of param and no autocomplete either.

image First one autocompletes fine. image the second one does not allow for auto complete and does not throw a type error.

image this is the object that it expects

Reproduction

Have an endpoint that expects multiple query params and try to fill in more then 1 in the options payload.

Expected result

Type safety when typing multiple

Checklist

Noahkoole avatar Jan 30 '24 14:01 Noahkoole

With further investigating, this issue was introduced in the 0.9.0next. Which I downloaded for the middleware, will post under that thread

Noahkoole avatar Jan 30 '24 14:01 Noahkoole

This seems to exist in 0.8.x as well. Will investigate a fix

drwpow avatar Feb 14 '24 03:02 drwpow

So I think this may have something to do with how TypeScript’s extends keyword works internally—it was originally designed for class inheritance, and thus it doesn’t prevent adding additional properties to the base, so long as the required keys are provided. So in your example, it will allow loffeeeqfqnk because it’s met its base requirements.

To be honest I’m not sure how to easily fix this—only allow the explicit parameters in the OpenAPI schema, and no more. I didn’t dig up any hints in the official TS docs on conditional types, but perhaps some TS wizards reading this would be so kind as to investigate a fix? 🙏

drwpow avatar Feb 14 '24 04:02 drwpow

Hey @drwpow,

So it seems like there's two issues here.

One is that the params can have arbitrary keys outside of the specified ones. I found that TypeScript 5.4 will be adding a new intrinsic type called NoInfer that might help with this. I put together a small example in the TypeScript playground to demonstrate this. Not 100% sure if it will be as easy as just wrapping the init parameter's type from ClientMethod in NoInfer, but that might work once TypeScript 5.4 is available and able to be used as a minimum version for this package.

The other issue is that once you add one optional parameter to the request, the rest of the optional parameters do not show up in autocomplete. This issue seems specific to version ^0.9.0 of openapi-fetch. I can look into that and make a PR if I find anything.

Edit: Here's some screenshot comparisons of 0.9.x and 0.8.x. This is using the cat facts api that is in the react-query example in this repo.

Example with 0.9.2: image

Example with 0.8.2: image

gittgott avatar Feb 19 '24 15:02 gittgott

I confirm I have both problems 🥲 I also tested in both 8 and 9, and the problem is present. It defeats a little the principle of implementing typescript. I can try the noInfer but I don't know how

clementAC avatar Apr 11 '24 16:04 clementAC

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

github-actions[bot] avatar Aug 06 '24 12:08 github-actions[bot]

@gittgott good idea.

I wrapped the init parameter in NoInfer and it does seem to resolve the issue. So a practical workaround for those on TS 4.5 would be to copy the type definitions for Client and ClientMethod into your own code and export the generated client as that type.

 import { FetchResponse, MaybeOptionalInit, Middleware } from "openapi-fetch"
import {
  HasRequiredKeys,
  HttpMethod,
  MediaType,
  PathsWithMethod,
} from "openapi-typescript-helpers"

type ClientMethod<
  Paths extends Record<string, Record<HttpMethod, {}>>,
  Method extends HttpMethod,
  Media extends MediaType,
> = <
  Path extends PathsWithMethod<Paths, Method>,
  Init extends MaybeOptionalInit<Paths[Path], Method>,
>(
  url: Path,
  ...init: NoInfer<
    HasRequiredKeys<Init> extends never
      ? [(Init & { [key: string]: unknown })?] // note: the arbitrary [key: string]: addition MUST happen here after all the inference happens (otherwise TS can’t infer if it’s required or not)
      : [Init & { [key: string]: unknown }]
  >
) => Promise<FetchResponse<Paths[Path][Method], Init, Media>>

export interface Client<Paths extends {}, Media extends MediaType = MediaType> {
  /** Call a GET endpoint */
  GET: ClientMethod<Paths, "get", Media>
  /** Call a PUT endpoint */
  PUT: ClientMethod<Paths, "put", Media>
  /** Call a POST endpoint */
  POST: ClientMethod<Paths, "post", Media>
  /** Call a DELETE endpoint */
  DELETE: ClientMethod<Paths, "delete", Media>
  /** Call a OPTIONS endpoint */
  OPTIONS: ClientMethod<Paths, "options", Media>
  /** Call a HEAD endpoint */
  HEAD: ClientMethod<Paths, "head", Media>
  /** Call a PATCH endpoint */
  PATCH: ClientMethod<Paths, "patch", Media>
  /** Call a TRACE endpoint */
  TRACE: ClientMethod<Paths, "trace", Media>
  /** Register middleware */
  use(...middleware: Middleware[]): void
  /** Unregister middleware */
  eject(...middleware: Middleware[]): void
}

export api  = createClient<paths>(...) as Client<paths>

For backwards compatibility: I wont pretend to understand it, but I saw the following helper type in the tanstack-query codebase, which achieves the same result for me.

type NoInfer<T> = [T][T extends any ? 0 : never]

SebKranz avatar Aug 07 '24 15:08 SebKranz

Thanks all for the detailed info and debugging. This bug is top-of-mind for us, and does need to be fixed. Bugs like this are what’s still keeping this library at 0.x rather than 1.0, especially tricky type bugs like this. Fortunately I think we’re at the end of the biggest ones, but have a few significant ones remaining like this.

PRs are welcome, of course! This is a major bug that does need addressing, as so many of you have pointed out. I personally can’t take this on for the next couple months, but will drop what I’m doing at any time to review PRs that address this.

But beyond a simple fix, one thing I’ve realized we’re missing currently is more robust *.test-d.ts tests that take full advantage of Vitest’s type utils. We have a few tests like this, yes, but I’m proposing taking all of the type checks out of the main test and almost completely separating runtime and type tests. Because we’re missing granular assertions that are leading to bugs like this as the TS “inference chain” gets other improvements.

This doesn’t have to happen in the same fix, it could be its own PR, but I think this change in testing would help fixing these a lot less daunting.

drwpow avatar Aug 14 '24 10:08 drwpow

I'm not able to reproduce this in [email protected]. If anyone is still seeing this issue (specifically, the "only the first query param is type checked" issue), could you please provide a more complete reproduction case including your tsconfig.json? I'd be interested to look into this.

ngraef avatar Aug 14 '24 15:08 ngraef