-
Notifications
You must be signed in to change notification settings - Fork 25
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
Issues with generating OpenAI client in zod #12
Comments
got it, feel free to send a PR 🙏 |
@astahmer happy to do that, if only had some understanding of where does the error come from. any hints? |
Hi @astahmer , @mhaligowski ! I think the problem is, that I could think of some other solutions:
// <ApiClient.get>
get<Path extends keyof GetEndpoints, TEndpoint extends GetEndpoints[Path]>(
path: Path,
...params: MaybeOptionalArg<TEndpoint["parameters"]>
): Promise<TEndpoint["response"]> {
return this.fetcher("get", this.baseUrl + path, params[0]) as Promise<TEndpoint["response"]>
}
// <ApiClient.get>
get<Path extends keyof GetEndpoints, TEndpoint extends GetEndpoints[Path]>(
path: Path,
...params: MaybeOptionalArg<z.infer<TEndpoint["parameters"]>>
): Promise<z.infer<TEndpoint["response"]>> {
return this.fetcher("get", this.baseUrl + path, params[0])
.then((unknownResponse) => EndpointByMethod["get"][path]["response"].parse(unknownResponse));
} I think from a zod-perspective, choice 2 would be the better choice: eventhough it is more "opionionated", it would ensure that the received response is actually correct, while in choice one it's possible to receive a wrong payload, but TypeScript would think the payload would ever be the correct type. On the other hand, the fetcher implementation must make sure, that if it receives a status code that it does not want to parse (like http 400), it rejects the promise, so that the const api = createApiClient((method, url, params) =>
fetch(url, { method, body: JSON.stringify(params) })
.then((res) => res.ok ? res.json() : throw new Error("invalid response")),
); Option number 3 would be to change the return type of the In any case I think, type Fetcher = (
method: Method,
url: string,
parameters?: EndpointParameters | undefined,
) => Promise<unknown>; What do you think? |
hey, I think option 1 (type casting) looks like a quick & easy solution here I feel like using a runtime solution (in option 2) should be done in the fetcher itself tho, so that anyone can opt-in to this kind of behaviour the point of this "headless" library was to not be opinionated on the underneath fetcher being used (fetch, axios, ky, etc) and how (validating input/output/both/none), as said in this issue I'm completely open to having a predefined fetcher implementation that satisfies most people's need even better, if we start having predefined fetcher implementation if we could (through a config option/CLI flag) specify which one to use ( all that said, I definitely don't want to repeat the option 3, I don't understand why this is needed, doing that would nullify most of the typings benefits from |
@astahmer : I created a pull request with option 1. If there is anything I should change, please let me know. Option 3 was a lousy workaround only to make the generated file compile and also make the consumer of the |
Thanks @astahmer for merging my pull requests. I tested the newest version with my own api description and no more compile error arises! 🥳 |
awesome ! thank you for the PRs 🙏 |
Great tool, incredibly helpful!
I am having issues when generating zod types for the OpenAI schema definition, available here: https://github.com/openai/openai-openapi/blob/master/openapi.yaml.
The generation works correctly, but when running
tsc
, I'm seeing issue like this:Interestingly, it doesn't have issues with the first method in the
ApiClient
class. I am running TypeScript 5.0.2, for that matter,typed-openapi
isd on 0.2.0.The text was updated successfully, but these errors were encountered: