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

fix(codegen): don't surface error responses as method return types #674

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

erunion
Copy link
Member

@erunion erunion commented Jul 14, 2023

🚥 Fixes #612

🧰 Changes

This fixes a problem in codegen where we're documenting 400 and 500 status code families as being possible return types for SDK methods which is not accurate as when we encounter these status codes we throw a FetchError. This is causing codegen's responses for these methods to effectively be typed as unknown as noted here1.

Unfortunately for us TypeScript doesn't offer any way to document that a method may throw an exception, and what that exception contains, so we need to fallback to a JSDoc @throws annotation. Not great but other than completely removing those error response types from the compiled SDK, which I don't want to do, we don't have any other option.

I have also cleaned up another very minor thing in this work with regard to typed method overloads that we're creating for methods that have an optional body or metadata. In these cases Because the first argument of the method can be either body or metadata we're creating a typed TS overload2 for each alternative. We were adding a docblock to each of these overloads but in actuality IDE Intellisense only ever picks up the docblock from the first method so adding the same docblock not only increases the size of the SDK we're generating but also fills the SDK with documentation that just isn't being used.

🧬 QA & Testing

The status code work is pretty well tested here with the SDK fixture snapshots we've got but for the docblock work you can see how the docblock from the first overload of this method is being picked up by Intellisense:

Screen Shot 2023-07-14 at 10 38 59 AM

For comparison, if we add that docblock to the last method it doesn't get picked up:

Screen Shot 2023-07-14 at 10 50 16 AM

Footnotes

  1. https://github.com/readmeio/api/issues/612#issuecomment-1633949991

  2. https://www.typescriptlang.org/docs/handbook/2/functions.html#function-overloads

@erunion erunion added bug Something isn't working refactor Issues about tackling technical debt area:core Issues related to `core`, which is the package that powers the SDKs at runtime labels Jul 14, 2023
@erunion erunion marked this pull request as ready for review July 14, 2023 17:59
@erunion erunion requested review from Dashron, kanadgupta, a team and domharrington and removed request for a team July 14, 2023 18:52
Copy link
Member

@domharrington domharrington left a comment

Choose a reason for hiding this comment

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

LGTM!

@erunion erunion merged commit cea1e7f into main Jul 17, 2023
6 checks passed
@erunion erunion deleted the fix/codegen-error-return-types branch July 17, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core Issues related to `core`, which is the package that powers the SDKs at runtime bug Something isn't working refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK generated by cli has incorrect return types
3 participants