Add support for grouping nested operations as class by operation id

[flow96]

Solves #2435 and #2437

A new sdk config has been added: groupByOperationId.

When groupByOperationId: true and asClass: true is set on the sdk plugin then the class generation will be based on the operation Id and endpoints won't be overridden by incorrect class re-usage.

Example config:

// openapi-ts.config.ts
export default defineConfig({
  input: './schema.json',
  output: './client',
  plugins: [
    '@hey-api/client-fetch',
    {
      asClass: true,
      groupByOperationId: true,
      name: '@hey-api/sdk',
    },
  ],
});

Example schema:

// schema.json
{
  "openapi": "3.0.0",
  "paths": {
    "/v1/domain1/fn1": {
      "get": {
        "operationId": "v1.tenants.providers.list",
        "responses": {
          "200": {
            "description": ""
          }
        }
      }
    },
    "/v1/domain2/fn2": {
      "get": {
        "operationId": "v1.tenants.providers.get",
        "responses": {
          "200": {
            "description": ""
          }
        }
      }
    },
    "/v2/domain1/fn3": {
      "get": {
        "operationId": "v2.tenants.providers.list",
        "responses": {
          "200": {
            "description": ""
          }
        }
      }
    },
    "/v2/domain2/fn4": {
      "get": {
        "operationId": "v2.tenants.providers.get",
        "responses": {
          "200": {
            "description": ""
          }
        }
      }
    }
  }
}

Result

// sdk.gen.ts
class V1TenantsProviders {
    public static list<ThrowOnError extends boolean = false>(options?: Options<V1TenantsProvidersListData, ThrowOnError>) {
        return (options?.client ?? _heyApiClient).get<V1TenantsProvidersListResponses, unknown, ThrowOnError>({
            url: '/v1/domain1/fn1',
            ...options
        });
    }
    
    public static get<ThrowOnError extends boolean = false>(options?: Options<V1TenantsProvidersGetData, ThrowOnError>) {
        return (options?.client ?? _heyApiClient).get<V1TenantsProvidersGetResponses, unknown, ThrowOnError>({
            url: '/v1/domain2/fn2',
            ...options
        });
    }
}

class V1Tenants {
    static providers = V1TenantsProviders;
}

export class V1 {
    static tenants = V1Tenants;
}

class V2TenantsProviders {
    public static list<ThrowOnError extends boolean = false>(options?: Options<V2TenantsProvidersListData, ThrowOnError>) {
        return (options?.client ?? _heyApiClient).get<V2TenantsProvidersListResponses, unknown, ThrowOnError>({
            url: '/v2/domain1/fn3',
            ...options
        });
    }
    
    public static get<ThrowOnError extends boolean = false>(options?: Options<V2TenantsProvidersGetData, ThrowOnError>) {
        return (options?.client ?? _heyApiClient).get<V2TenantsProvidersGetResponses, unknown, ThrowOnError>({
            url: '/v2/domain2/fn4',
            ...options
        });
    }
}

class V2Tenants {
    static providers = V2TenantsProviders;
}

export class V2 {
    static tenants = V2Tenants;
}

Usage:

// Example usage
V1.tenants.providers.list();
V2.tenants.providers.list();

When setting instance: "MySdk" the result will look like this

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[changeset-bot]

⚠️ No Changeset found

Latest commit: ec7cb7a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

@flow96 is attempting to deploy a commit to the Hey API Team on Vercel.

A member of the Team first needs to authorize it.

[mrlubos]

oh no

[flow96]

oh no

Most of that is just the snapshots of the tests 😄
What about the conflicts? Should I sync the fork and rebase?

[mrlubos]

@flow96 yes please. I changed those files a lot, you might want to just apply your changes anew

[mrlubos]

@flow96 Why didn't you use this pattern? #2435 (comment)

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2450

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2450

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2450

commit: ec7cb7a

[flow96]

@flow96 Why didn't you use this pattern? #2435 (comment)

@mrlubos I was thinking about that too, but you also mentioned that people currently rely on the current behavior, so I thought it might be better to first introduce it as an optional configuration value with default false. Since the current implementation uses mainly the tags and the new one fully relies on the operationIds - some people might complain.

In my opinion the new output (based on the operationIds) should be the default but I am with you, that there are probably people that rely on the old/current behavior, so changing the default might not be a good choice. But that's just my thoughts, in the end you have to decide :)

If you prefer to change the default behavior, I can change it like that and call the config property something like legacyClassGeneration or whatever you prefer 😄

[mrlubos]

No I want this to be the default, it's a question of semantics and providing clear configuration options.

You talk about operation IDs, but those are optional keywords in OpenAPI. What happens if there's no operation ID value? I didn't look at the code yet. Just want your thoughts, maybe it'll help with naming.

[flow96]

Ok, thanks for the input.
I actually made it throw an error if there is no operation Id, so the generation currently fully relies on the operation IDs since they allow us to define unique nested classes - thus the configuration name groupByOperationId.

With tags we have the same issue as before: Endpoints would be consolidated/overwritten if they share the same method name.

E.g. the following two endpoints:

path: /tenants/providers/list
tags: [Tenants, Providers]
// and
path: /providers/list
tags: [Providers]

The sdk would end up with a single Providers class, that only contains one list method

class Providers {
    public list() {...}
}

Creating nested classes with tags is not easy, because we also cannot rely on the order of the tags to create nested classes. Ordering them ourself might create incorrect nestings.

So my suggestion would be to rather try to create a nesting based on the path instead of the tags.

Maybe I am wrong here but these were my thoughts during the implementation - that's why I thought it makes more sense to go this way.
What do you think regarding the tags problem? Should we accept the fact that methods are overwritten in cases when no operation ID is given?

Sorry for the long text

[mrlubos]

Not long at all! I'm going to be obtuse because I didn't fully follow the previous issues, but how do we end up with a single method in your example? We're able to uniquely identify an operation by its path + method, why is that ever an issue?

create a nesting based on the path instead of the tags

I believe this is the way. You can't rely on tags alone, and they're also optional.

accept the fact that methods are overwritten in cases when no operation ID is given

This will never be acceptable. If I define 10 operations in my spec, I expect 10 (or more) methods in my SDK, no matter what. If I'm getting fewer than that, and it sounds like that's what you're saying, that's not acceptable.

Speaking of...

the generation currently fully relies on the operation IDs

There must be a fallback. This configuration is too brittle otherwise and will be confusing. Imagine you successfully use this feature, and then your API team adds an endpoint without operation ID. Now your client is blocked because it crashes. This issue is the biggest one to resolve imo