Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task
Noahkoole opened this issue Jan 30, 2024 · 9 comments
Open
1 task

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

Noahkoole opened this issue Jan 30, 2024 · 9 comments
Labels
bug Something isn't working good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue!

Comments

@Noahkoole
Copy link

Noahkoole commented Jan 30, 2024

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 Noahkoole added bug Something isn't working openapi-fetch Relevant to the openapi-fetch library labels Jan 30, 2024
@Noahkoole
Copy link
Author

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

@Noahkoole Noahkoole mentioned this issue Jan 30, 2024
3 tasks
@drwpow
Copy link
Contributor

drwpow commented Feb 14, 2024

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

@drwpow drwpow reopened this Feb 14, 2024
@drwpow
Copy link
Contributor

drwpow commented Feb 14, 2024

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 drwpow added PRs welcome PRs are welcome to solve this issue! good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project labels Feb 14, 2024
@gittgott
Copy link

gittgott commented Feb 19, 2024

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
Copy link

clementAC commented Apr 11, 2024

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

Copy link
Contributor

github-actions bot commented Aug 6, 2024

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 github-actions bot added the stale label Aug 6, 2024
@SebKranz
Copy link

SebKranz commented Aug 7, 2024

@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]

@github-actions github-actions bot removed the stale label Aug 8, 2024
@drwpow
Copy link
Contributor

drwpow commented Aug 14, 2024

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
Copy link
Contributor

ngraef commented Aug 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue!
Projects
None yet
Development

No branches or pull requests

6 participants