openapi-ts / openapi-typescript

Generate TypeScript types from OpenAPI 3 specs
https://openapi-ts.dev
MIT License
5.82k stars 466 forks source link

Query param type safety only works for the first param. #1525

Open Noahkoole opened 9 months ago

Noahkoole commented 9 months ago

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 commented 9 months ago

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

drwpow commented 8 months ago

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

drwpow commented 8 months ago

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? 🙏

gittgott commented 8 months ago

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
clementAC commented 6 months ago

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

github-actions[bot] commented 2 months ago

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.

SebKranz commented 2 months ago

@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]
drwpow commented 2 months ago

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.

ngraef commented 2 months ago

I'm not able to reproduce this in openapi-fetch@0.11.1. 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.