-
-
Notifications
You must be signed in to change notification settings - Fork 532
Polymorphic status code responses: ideas wanted! #1128
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
Comments
I like option B. It’s not breaking (which in the current alpha phase of the package doesn’t really matter, but still…). Also, there’s probably more info contained within edit: I’ll use the library in a react-router/Remix project, so having more than just the status code available in an explicit manner would certainly be super helpful. Just curious: Why do you not want to use generics? |
True—breaking changes don’t matter much in pre-1.0. But I think the API / usability is everything, and I do think sweating the details matter! I should have put more emphasis on “easy to use” more than “breaking changes.” I also would rather err on the side of “happy path is easy/terse; complex path is mildly annoying“ rather than “everything is equally mildly annoying.” 😄
The main README has a note on this, but that’s a path to errors. Imagine the lowest-common-denominator in a codebase: not yourself, but either someone that doesn’t know better, or someone who is trying to shortcut everything. Imagine having the wrong generic, or simply providing |
So I’m leaving this issue open for now, because I want to investigate this further. But I think for various reasons, we’ll be going forward with Option B mostly because that’s lighter-weight and easier to get type inference working for. Currently the only thing missing in |
Update on this: the |
I am very interested in this, and just wanted to add my support for going forward with Option B! In the meantime, we use something of the sort to get the right types based on the status code: if (response.error) {
let error;
switch (response.response.status) {
case 400:
error = response.error as paths["/rtm/notes/create"]["post"]["responses"]["400"]["content"]["application/ld+json"];
break;
case 422:
error = response.error as paths["/rtm/notes/create"]["post"]["responses"]["422"]["content"]["application/ld+json"];
break;
// etc...
} It's not nearly as neat, but it kind of "works". 🤷♂️ |
I also support option B, I think checking status codes is a good practice anyway. I'm working with this api:
|
This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed. |
This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem. |
Say your endpoint returned entirely different shapes for
200
,201
,404
, and500
responses. The current version of this library would assigndata
to200
, anderror
to500
, while ignoring the201
and404
responses altogether.While this is the pattern that most endpoints follow—usually an endpoint will return either
201
or200
on success, but not both, which this library supports today—it still is a theoretical hiccup for valid schemas wanting to use this library.Supporting this is unclear, though! How could this library provide support for multiple responses, without the use of generics?
Option A: Response object (breaking change)
Note: TS would throw an error on invalid keys, e.g.
res[0]
. Worth noting this is typed as an object with numeric indices, and not just a standard array.Option B:
response
discriminator (non-breaking change)Option C: ???
If you have another proposal, please submit one below!
The text was updated successfully, but these errors were encountered: