Skip to content
This repository was archived by the owner on May 22, 2023. It is now read-only.

optional 2nd parameter #10

Merged
merged 1 commit into from
Apr 6, 2023
Merged

optional 2nd parameter #10

merged 1 commit into from
Apr 6, 2023

Conversation

fergusean
Copy link
Contributor

@fergusean fergusean commented Apr 5, 2023

Changes

I think this solves for openapi-ts/openapi-typescript#1127, though it's not very pretty. Feel free to reject it solely on its visual appeal 😅. It shouldn't jeopardize types since the cast is only applied to the fallback empty object.

In retrospect, I mostly agree with your counterpoint. I just happened to first use it on a parameterless endpoint, but most at least have conditional parameters.

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Checklist

  • Tests updated
  • README updated

@fergusean
Copy link
Contributor Author

Also -- much thanks for your work on this & https://github.com/drwpow/openapi-typescript! You're helping my dream of auto-gen'ed OpenAPI YAML from the backend + auto-gen'ed API client on the frontend become a reality 😄

Copy link
Owner

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

This is amazing, thank you!

I’ll test this a little bit as there are some tests I want to add, and I’ll make sure this doesn’t throw anything off. But “visual appeal” is not something that will be possible with all the TypeScript shenanigans in this library 😄

@drwpow drwpow merged commit f4dd043 into drwpow:main Apr 6, 2023
@drwpow
Copy link
Owner

drwpow commented Apr 6, 2023

Hm, no, unfortunately this does break the type inference—the options?: lets you omit required params for all methods. We’ll need a test to ensure that this is working as intended (note that the project has TypeScript tests—we have an instance where a // @ts-expect-error will actually throw if there is NOT a TS error that follows. We’ll need the same thing to test here—test than an endpoint missing required params must have // @ts-expect-error to pass)

@fergusean
Copy link
Contributor Author

ahhhh, that makes sense. I hadn't considered that. back to the drawing board! 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants