hey-api / openapi-ts

🚀 The OpenAPI to TypeScript codegen. Generate clients, SDKs, validators, and more. Support: @mrlubos
https://heyapi.dev
Other
1.37k stars 106 forks source link

Legacy to standalone client migration #837

Open tervay opened 3 months ago

tervay commented 3 months ago

Description

First I want to say thanks to the developers of this project, I appreciate the direction the project is going.

We have a large API that is based entirely on path parameters, e.g. /pets/{petId}. A sample OpenAPI client can be found here, or here:

openapi: 3.0.0
info:
  title: Widget Service
  version: 0.0.0
tags: []
paths:
  /pets/{petId}:
    get:
      operationId: Pets_read
      parameters:
        - name: petId
          in: path
          required: true
          schema:
            type: integer
            format: int32
      responses:
        '200':
          description: The request has succeeded.
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pet'
components:
  schemas:
    Pet:
      type: object
      required:
        - petId
      properties:
        petId:
          type: integer
          format: int32

Suppose we have the following openapi-ts.config.ts :

export default defineConfig({
    client: "<my client>",
    input: "sample.yaml",
    output: {
        format: "prettier",
        path: "./app/api/client/",
        lint: "eslint"
    },
})

The generated code changes pretty significantly between the legacy fetch client and the current @hey-api/client-fetch client:

import { petsRead } from '~/api/client';

// with legacy `fetch`:
const pet = await petsRead({ petId: 1 });

// with current `@hey-api/client-fetch`:
const pet = await petsRead({ path: { petId: 1 }});

Note the extra path parameter here. Is there any way to avoid having to specify this on the modern fetch client? It's pretty verbose and clunky, since every single call requires { path: { ... } } now.

mrlubos commented 3 months ago

@tervay Not at the moment, which is why it was created as a separate package. How many instances are we talking about?

tervay commented 3 months ago

@mrlubos Probably 50ish endpoints, which is why I didn't just wrap it. If it was say five endpoints I'd just write some helper functions.

mrlubos commented 3 months ago

That's going to be tough. Maybe your API has an explicit requirement for path parameters only, but what if you were to add query parameters in the future? The return types are also different btw, it's not just pet. I'd say gradual migration is still the easiest option here https://heyapi.vercel.app/openapi-ts/migrating.html#useoptions-true, but if you have a proposal for functionality, let me know (there are certainly ways to get it working with path params only APIs)

tervay commented 3 months ago

I'd like to imagine it should be possible to semi-intelligently merge query/path parameters into one blob; that is, suppose we had:

export type PetsReadData = {
  path: {
    petId: number;
  };
  query: {
    full: boolean;
  };
};

It could be entirely possible to merge these into one options object:

export type MergedData = {
  petId: number;
  full: boolean;
};

If there's a name collision then... 🤷 ? Emit a warning? Fail? Not sure. Another option could be added to either merge or separate path/query param objects.

I suppose the library then needs to track which are query and which are path to send it off to fetch, though. Sounds annoying, so maybe this isn't easy.

mrlubos commented 3 months ago

You described it perfectly in the last sentence. I will close this issue for now, but if anyone thinks there's a viable approach we should consider, we can re-open.

mrlubos commented 3 months ago

And I'm bringing it back. I think resolving https://github.com/hey-api/openapi-ts/issues/558 correctly will unblock this issue

mrlubos commented 3 months ago

Might need to handle useOptions=false in this release too