Skip to content
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

feat: open-api default response for dart #9709

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

midzelis
Copy link
Contributor

@midzelis midzelis commented May 23, 2024

This PR allows the nestjs controllers to be annotated with the status code specific responses, like @ApiCreatedResponse and @ApiOkResponse - and allows the use of different return types for each response. This works perfectly fine with the typescript client, however, the dart2 open-api generator will pick the type of the first 2xx response as the return type of the the operation - which is not correct if there are divergent types for each response code. To allow the the use of typescript to make use of the additional type information, and also support Dart as best we can - you can make use of the @ApiResponse{status: 'default', ... } annotation - this PR patches the api.mustasch dart template to use the default response's return type instead of the first 2xx response. The coding guidelines would be - if you are planning to different types for different response codes, also include a default response with a type that is the logical union of all the properties each type has. This doesn't give you as nice a type in the the response, but its the best that can be done for the Dart language, as it does not support union return types like typescript and other languages can.

Note - the Dart api generator will generate Dart methods for each operation - one that is typed, and one that uses a generic Response object that has the format <operation>WithHttpInfo. A dart client would ALWAYS be able to use the generically typed form of this method.

This has been split from: #8480

open-api/templates/mobile/api.mustache Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth submitting a PR upstream for this functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure - the result is not perfect (you still need to define a default response), but it might be better than how it is today. This was inspired by this change: OpenAPITools/openapi-generator#128 I'll submit a PR upstream a bit later and see if they want to merge it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is that lets see if they're willing to accept this upstream, and then if not we can consider whether we want to add this just for our solution. Imo this leaves us in a weird spot where we can use the annotations but only have partial support, I feel like we may just be better not using those annotations for both platforms.

@midzelis midzelis requested a review from jrasm91 May 24, 2024 20:58
@jrasm91 jrasm91 requested a review from zackpollard May 28, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants