Skip to content

Response type void when no content in schema. #1576

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
wants to merge 1 commit into from

Conversation

josstn
Copy link

@josstn josstn commented Jan 16, 2025

After this change response type is set to void when no content schema is provided, regardless of status code of response.

Before this change, it was only set to void for status code 204.

The spec (https://swagger.io/docs/specification/v3_0/describing-responses/) does not say that an empty response body is only valid for 204 responses, it is just used as an example.

After this change response type is set to void when no content schema is provided, regardless of status code of response.

Before this change, it was only set to void for status code 204.

The spec (https://swagger.io/docs/specification/v3_0/describing-responses/) does not say that an empty response body is is only valid for 204 responses, it is just used as an example.
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Jan 16, 2025

⚠️ No Changeset found

Latest commit: 8a136ac

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

Copy link

vercel bot commented Jan 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hey-api-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 8:48am

@mrlubos
Copy link
Member

mrlubos commented Jan 16, 2025

@josstn How does this impact the generated code? Why do you need to make this change?

@josstn
Copy link
Author

josstn commented Jan 16, 2025

@mrlubos The reason for this is that I have endpoints that return empty responses (void methods in my java source code) with response code 200. I want my generated typescript to reflect this properly.

I also believe this generally is the more correct solution. (Why should the return type not be void when the input schema states that there is no response content?)

@josstn
Copy link
Author

josstn commented Jan 16, 2025

I'm currently reviewing the diffs to the test-snapshots. As far as I can tell the main impact on generated code is that a lot of cases that previously was generated with type unknown now is typed with void.

One change I'm wondering about though is that in some cases a "DefaultError" type is replaced with a new generated response type that has type void for all status codes.

Example from react-query.gen.ts:
Before:

export const callWithResultFromHeaderMutation = (options?: Partial<Options<CallWithResultFromHeaderData>>) => {
    const mutationOptions: UseMutationOptions<unknown, DefaultError, Options<CallWithResultFromHeaderData>> = {
        mutationFn: async (localOptions) => {
            const { data } = await HeaderService.callWithResultFromHeader({
                ...options,
                ...localOptions,
                throwOnError: true
            });
            return data;
        }
    };
    return mutationOptions;
};

After:

export const callWithResultFromHeaderMutation = (options?: Partial<Options<CallWithResultFromHeaderData>>) => {
    const mutationOptions: UseMutationOptions<CallWithResultFromHeaderResponse, AxiosError<CallWithResultFromHeaderError>, Options<CallWithResultFromHeaderData>> = {
        mutationFn: async (localOptions) => {
            const { data } = await callWithResultFromHeader({
                ...options,
                ...localOptions,
                throwOnError: true
            });
            return data;
        }
    };
    return mutationOptions;
};

The generated CallWithResultFromHeaderError looks like this, so it's basically void:

export type CallWithResultFromHeaderErrors = {
    /**
     * 400 server error
     */
    400: void;
    /**
     * 500 server error
     */
    500: void;
};

export type CallWithResultFromHeaderError = CallWithResultFromHeaderErrors[keyof CallWithResultFromHeaderErrors];

@mrlubos
Copy link
Member

mrlubos commented Jan 16, 2025

Please read this thread, I don't believe there's a fool-proof way of declaring intent behind responses with no content OAI/OpenAPI-Specification#3536

@josstn
Copy link
Author

josstn commented Jan 16, 2025

Interesting, thanks for the pointer. My understanding then is that the spec currently does not specify what the behaviour should be, including for response code 204.

But it seems like some (most?) people think that the correct behaviour is that it should be unknown.

The suggested way in the linked thread of explicitly defining an empty response by setting schema: false also does not seem to be valid for Openapi spec version 3.0(?)

I also wondered if specifying "type": "null" or in schema could be a solution to signal the code generator to generate null as response type. It was easy to implement support for it in openapi-ts, but I see that "type": "null" is not in the OpenApi spec, even though it is in the json schema spec, so I guess that is out of the question also.

So then I guess we are left with no way of specifying a void (or null) response type.

@mrlubos
Copy link
Member

mrlubos commented Jan 16, 2025

In 3.0 you could use nullable. I believe there's a downside to this and its implementation-specific whether it gets handled as you're expecting. You may need to define type along with nullable, but that would defeat the purpose.

What you wrote elsewhere sounds right. There's no way to clearly define no content, however the specification for certain status codes makes it clear there must be no content, which is what I left as a TODO comment in the code you're changing.

Unless there's a better way, unknown is generally correct as it's the same as not describing a response. We could offer a config option to use void, though not sure how much interest there's in that. Another alternative would be to offer hooks into the parser and you could configure this yourself. Or we could add an extension field? You're the first one to bring this up!

@josstn
Copy link
Author

josstn commented Jan 17, 2025

Using nullable in 3.0 is as far as I can tell only valid when specifying some other type. It allows you to make a type such as string | null, but not just null.

I agree that unknown is probably the best default to have now. I actually think it could be very valuable if the openapi-ts parser had support for propagating any x- extension property from the input spec on to plugins so that people like me could make custom plugins do what I want. Then I could add something like an x-no-content: true extension to some response schemas, and then have a plugin in openapi-ts change the return type to void before the default plugins create the code.

I tried hacking this in yesterday, and could quite easily get the parser to propagate the extension into its model. I was also able to make a plugin that changed the response type to void before the sdk plugin was to create the code, but it did not help, the sdk plugin went on to make unknown the return type. It looks like it currently only allows response types to be references to types made by the "types" plugin, and anything else (like void or null) becomes unknown.

If there was a way a custom plugin could change the parsed model before the types.gen.ts gets generated, perhaps that would be a solution to this?

@mrlubos
Copy link
Member

mrlubos commented Jan 17, 2025

Yep, I can get behind this, it's a really good idea! Let me create an issue

@josstn
Copy link
Author

josstn commented Jan 17, 2025

Thanks, I'll close this PR then

@josstn josstn closed this Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants