Skip to content

Consider using codegen instead of generics #1779

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

Closed
1 task
RPGillespie6 opened this issue Jul 23, 2024 · 13 comments
Closed
1 task

Consider using codegen instead of generics #1779

RPGillespie6 opened this issue Jul 23, 2024 · 13 comments
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library

Comments

@RPGillespie6
Copy link

Description

Consider using codegen instead of generics for openapi-fetch. Generics seem to come with a host of problems and limitations, including:

  • Can't examine data structures in IDEs
    • For example in latest VSCode I can't easily examine the shape of POST bodies with F12 because of generics
  • Can't forward doc strings in IDEs
    • If I hover over a client.POST call, I can't read the doc string provided in the openapi spec description field because of generics
  • Extremely esoteric typescript (which, due to its difficulty to understand, apparently results in bugs like params always required #1778 and Undefined properties on body object are not flagged if required fields are filled in #1769)
  • Clients display methods like PATCH in intellisense for which there are no paths

You already have to do a codegen step for openapi-typescript, why not just cut out the middleman and do:

npx openapi-fetch path/to/petstore.yaml -o openapi-fetch-petstore.ts

Then you can do:

import createClient from "./openapi-fetch-petstore";

...

I don't think it would be significantly bigger because most of the codegen will be type declarations, overloads, etc which get stripped out. And I'm sure there's some shared code that could be leveraged from openapi-typescript

Checklist

@RPGillespie6 RPGillespie6 added enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library labels Jul 23, 2024
@drwpow
Copy link
Contributor

drwpow commented Jul 23, 2024

Thanks for suggesting but if you read all the about sections of why this project exists and goals it solves, this entire philosophy is to specifically NOT codegen. To change this would mean rewriting everything.

If you need codegen, that’s great! There are lots of other options that provide this. But those are unrelated to this project.

@drwpow drwpow closed this as completed Jul 23, 2024
@RPGillespie6
Copy link
Author

I'm not talking about swagger-style codegen. I'm talking about metaprogramming as an alternative to generics.

The API would still be the same, i.e.

import createClient from "openapi-fetch-petstore";

const client = createClient();

client.POST("/store/order", {
    body: {
        id: 0,
    },
})

It would just resolve all of the problems that generics are currently causing.

@RPGillespie6
Copy link
Author

RPGillespie6 commented Jul 23, 2024

To change this would mean rewriting everything.

Yeah I think this openapi-fetch does need a rewrite. It's broken as to be unusable at the moment (in terms of type safety) and hasn't even reached v1.0. I say cut your losses and do it the right way

@drwpow
Copy link
Contributor

drwpow commented Jul 23, 2024

I understand your frustration; there have been some issues with the last couple releases. But to pose the entire library needs a rewrite and to “cut my losses“ isn’t a fair representation of what this library has achieved over many years of advancements and how many people use it (and is running in production in several stacks; I’m just sorry it doesn’t work for yours at the moment).

If you’d like to create your own codegen library there’s always room at the table for more ideas and input! But rewriting this isn’t necessary. A few bugs don’t necessitate a rewrite 🙂. That’s why it’s still at 0.x, mind you.

@RPGillespie6
Copy link
Author

RPGillespie6 commented Jul 23, 2024

Ok, well thanks for the rationale. I'll keep an eye on this project. I really like the idea of this project and I was sold on it, but after playing with it for a few days I've found a few pain points that make it not quite ready for prime time (for my needs, at least) due to the bugs.

In the meantime I think I'll probably roll my own API-compatible codegen implementation of openapi-fetch, which I'll link here when it's ready. Maybe I can convince you yet of the benefits of metaprogramming, which can do things generics can't do (mainly around IDE QoL things like being able to jump to definitions of payloads, view doc strings on hover, etc)

@RPGillespie6
Copy link
Author

RPGillespie6 commented Jul 24, 2024

Actually, looking through the code, I don't think it would need a complete rewrite to achieve what I'm talking about. I think you could dogfood the openapi-typescript types and also dogfood index.js in openapi-fetch and all you would need to do is write small generator to generate the things in index.d.ts currently being handled by generics.

So, for example, instead of:

export default function createClient<Paths extends {}, Media extends MediaType = MediaType>(
  clientOptions?: ClientOptions,
): {
  /** 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;
};

You would generate something similar, but not exactly, like this (if petstore yaml were the input):

interface Client {
    POST(path: "/pet", body: components["schemas"]["Pet"]): Promise<PetResponse>;
    POST(path: "/user", body: components["schemas"]["User"]): Promise<UserResponse>;
    POST(path: "/user/createWithArray", body: components["requestBodies"]["UserArray"]["content"]["application/json"]): Promise<BlahResponse>;
    POST(path: "/store/order", body: components["schemas"]["Order"]): Promise<OrderResponse>;
}

export function createClient(clientOptions?: ClientOptions): Client;

And then the beauty if this is that not only is the typescript super easy to understand, it fixes all the issues I've been encountering:

  • Intellisense now displays only the available methods instead all methods
  • Jump to definition starts working again in VSCode (currently breaks arbitrarily depending on the component)
  • The two issues I mentioned above are also fixed

I know this is an issue with generics, because if I just extract the types from petstore.d.ts and use them in functions, everything works perfectly. Stuff only starts breaking in the context of generics (i.e. client.POST("", { this part }))

I don't think this is completely unreasonable, because openapi-typescript also requires a type generation step, and this approach would continue to dogfood those generated types, but with some additionally generated fetch wrappers.

I might fork the repo this weekend and try my hand at it, I don't think it would take more than a couple hundred lines of code to generate the parts of index.d.ts that are currently being handled by generics.

@RPGillespie6
Copy link
Author

RPGillespie6 commented Aug 19, 2024

@drwpow I made typed-fetch as a reference implementation in case you missed my comment in the other issue. I could see a future where:

npx openapi-typescript path/to/petstore.yaml -o petstore.d.ts

Generates not just the component types, but also the init types and the client interface as well like I've done here: https://github.com/RPGillespie6/typed-fetch/blob/main/examples/petstore-openapi.d.ts#L366

I think that would vastly simplify the implementation of openapi-fetch such that you wouldn't need deeply nested generics and you could use an actual debugger for debugging. It would also eliminate the need for openapi-fetch's index.d.ts and allow you to focus on just the implementation file.

@drwpow
Copy link
Contributor

drwpow commented Aug 20, 2024

That’s awesome, thanks for making! I’d be happy to link that in the docs of alternate solutions.

Again, this whole project was created as a response to codegen. Yes, generating TypeScript types is, codegen, but it was done so that a generated SDK wouldn’t have to be created. Those tend to be bulky, and slow, and harder to maintain (to your point, they don’t have to be, but if you look at how Swagger codegen has gone, and other codegen projects that have more open issues than this project, you can at least see a pattern). I’m saying this not to discourage you at all—I think you creating this is great, and I want to see you succeed (and prove me wrong!). I want codegen to be in a better place. It’s just that what you’re describing, historically, has been an anti-goal of these libraries from the beginning. And that’s the beauty of open source—just providing more options available to people.

@RPGillespie6
Copy link
Author

RPGillespie6 commented Aug 20, 2024

That’s awesome, thanks for making! I’d be happy to link that in the docs of alternate solutions.

Sure, I would appreciate that, but if openapi-fetch ever reaches 1.X I'll probably discontinue maintaining typed-fetch just because I'd rather centralize on battle tested tooling that lots of people use vs. fracture the space further

but it was done so that a generated SDK wouldn’t have to be created.

Just curious, but do you consider typed-fetch to be a generated SDK based on what you see in https://github.com/RPGillespie6/typed-fetch/blob/main/examples/petstore-openapi.d.ts? IMO, it's nearly identical to openapi-typescript in what it generates with the addition of some helper types for fetch request and response so typed-fetch.ts can remain openapi spec agnostic while also not needing to rely on complex difficult-to-debug generics. typed-fetch doesn't generate any actual code, it generates 100% types just like openapi-typescript. I guess I'm just not understanding which line you are not willing to cross in terms of codegen since as you admitted, openapi-typescript is TS type codegen already. i.e. Can you point to which part of petstore-openapi.d.ts crosses a line that you are not willing to cross?

Also note the shape of the types in typed-fetch are somewhat arbitrary and could be changed. For example

type ComponentSchemaAddress = {
    /** Example: Palo Alto */
    city?: string;
    /** Example: CA */
    state?: string;
    /** Example: 437 Lytton */
    street?: string;
    /** Example: 94301 */
    zip?: string;
}

could be changed to

type components = {
    "schemas": {
        "Address": {...}
    }
}

without affecting the functionality of typed-fetch. I just flattened it to simplify type pick-and-place.

@drwpow
Copy link
Contributor

drwpow commented Aug 20, 2024

Sure, I would appreciate that, but if openapi-fetch ever reaches 1.X I'll probably discontinue maintaining typed-fetch just because I'd rather centralize on battle tested tooling that lots of people use vs. fracture the space further

I wouldn’t personally worry about “fracturing the space;” that’s thinking in product terms. Open Source is meant to explore new ideas, and put more ideas out into the world. The more, the merrier.

Just curious, but do you consider typed-fetch to be a generated SDK based on what you see in https://github.com/RPGillespie6/typed-fetch/blob/main/examples/petstore-openapi.d.ts? … i.e. Can you point to which part of petstore-openapi.d.ts crosses a line that you are not willing to cross?

Running a CLI command to generate openapi-fetch after you’ve already generated your schema is the line I’m not willing to cross 🙂. Kudos for being able to only generate types, but that still puts it in a different category altogether and fundamentally changes the project.

FWIW, openapi-fetch was inspired by a private project where I generated types for an API just like you were doing. It broke all the time, we were constantly patching it. And if you didn’t generate the types juuuust so, the TS Language Server would actually time out traversing all that code and loading it into memory (I would have doubts that this approach would work on really really large schemas—not the generation part, but the TS inference part in the Language Server).

@RPGillespie6
Copy link
Author

Running a CLI command to generate openapi-fetch after you’ve already generated your schema is the line I’m not willing to cross 🙂

Ah, but that's not what typed-fetch does nor what I'm proposing. There is only ever 1 codegen step, and it's the step to generate the schema.

For openapi-fetch it would be:

npx openapi-typescript petstore.yaml -o petstore.d.ts

For typed-fetch the equivalent command is:

typed-fetch --openapi petstore.yaml --output petstore.d.ts

These commands are equivalent, and the only difference between them at the moment is that typed-fetch generates a few extra types in the schema to avoid needing generics later.

Both typed-fetch and openapi-fetch use codegen-free static implementations:

  1. openapi-fetch uses index.d.ts + index.js

  2. typed-fetch uses typed-fetch.ts

@RPGillespie6
Copy link
Author

RPGillespie6 commented Aug 20, 2024

I guess one argument could be that you don't want to pollute the "pure" openapi-typescript schema with extra types that would only ever be used by openapi-fetch, but you could always gate the extra types behind a flag:

npx openapi-typescript --openapi-fetch petstore.yaml -o petstore.d.ts

However, I'm currently of the opinion though that extra potentially unused types are not a big deal though because tsc will just ignore/discard them.

I would have doubts that this approach would work on really really large schemas—not the generation part, but the TS inference part in the Language Server

Do you have an example "really really large schema" you could share that I could use for testing typed-fetch (and seeing how TS language server behaves, etc)? Actually, I guess I could just generate a bogus huge schema. What would you consider "really really large"? 1000 endpoints? 10000?

@RPGillespie6
Copy link
Author

RPGillespie6 commented Aug 20, 2024

Ok, I generated some bogus petstore schemas: petstore-big-schemas.zip. One has ~1000 endpoints, the other has ~10000 endpoints.

With petstore1000.yaml intellisense is pretty much instant for me with both typed-fetch and openapi-fetch.

With petstore10000.yaml, typed-fetch took about ~6s to populate intellisense while openapi-fetch took ~2s to populate intellisense (so openapi-fetch is currently ~3x faster with intellisense for very large schemas!). I think I could restructure typed-fetch's generated schema a bit to improve intellisense performance. On the flip side though, openapi-typescript took ~16s to generate the d.ts, while typed-fetch only took ~4s to generate the .d.ts. So development velocity is impacted either way, unless the HTTP API you are using is super stable and never needs changes.

At any rate though, I would hope it's pretty rare people have http apis with 10000 endpoints (and either way, I couldn't get the TS server to timeout or freeze).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

No branches or pull requests

2 participants