Skip to content

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

Closed
drwpow opened this issue Mar 16, 2023 · 8 comments
Closed

Polymorphic status code responses: ideas wanted! #1128

drwpow opened this issue Mar 16, 2023 · 8 comments
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue! question Further information is requested stale

Comments

@drwpow
Copy link
Contributor

drwpow commented Mar 16, 2023

Say your endpoint returned entirely different shapes for 200, 201, 404, and 500 responses. The current version of this library would assign data to 200, and error to 500, while ignoring the 201 and 404 responses altogether.

While this is the pattern that most endpoints follow—usually an endpoint will return either 201 or 200 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)

const res = await get('/path/to/endpoint');
if (res[200]) {  }
else if (res[201]) {  }
else if (res[404]) {  }
else {  }

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)

const { data, error, response } = await get('/path/to/endpoint');
if (response.status === 200) {  }      // `data` is defined, has 200 shape
else if (response.status === 201) {  } // `data` is defined, has 201 shape
else if (response.status === 404) {  } // `error` is defined, has 404 shape
else {  }                              // `error` is defined, has 500 shape

Option C: ???

If you have another proposal, please submit one below!

@drwpow drwpow added PRs welcome PRs are welcome to solve this issue! question Further information is requested enhancement New feature or request labels Mar 16, 2023
@HerrBertling
Copy link

HerrBertling commented Apr 8, 2023

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 response than just the status code, so this would be a handy way to have the “meta” stuff separated from the actual data/error.

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?

@drwpow
Copy link
Contributor Author

drwpow commented Apr 11, 2023

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.” 😄

Why do you not want to use generics?

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 <any> everywhere. That’s effectively as good as not using TypeScript! And further, I believe generics are a last-resort when introspection fails. And within the realm of your OpenAPI schema, I think it’s more helpful to assume everything can be introspected. No errors, no shortcuts, and even better usability!

@drwpow
Copy link
Contributor Author

drwpow commented May 1, 2023

So v0.1.0 does somewhat solve this in that it creates unions of “good” and “bad” types. Theoretically, now, if you have different schemas for different error codes (like different shapes entirely), then that will now be reflected when you check { error }. Likewise for { data } (though that is much less-common to have multiple different responses for the same method that have wildly-varying shapes).

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 0.1.0 from making Option B possible is the response.status discriminator—currently it doesn’t associate specific statuses with specific response codes yet (but is very possible to do in an upcoming release)

@drwpow drwpow transferred this issue from drwpow/openapi-fetch May 22, 2023
@drwpow drwpow added the openapi-fetch Relevant to the openapi-fetch library label May 22, 2023
@drwpow
Copy link
Contributor Author

drwpow commented May 22, 2023

Update on this: the response.status discriminator support still hasn’t been added, and this issue will be complete once that’s in.

@EmilePerron
Copy link

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". 🤷‍♂️

@RobinClowers
Copy link

RobinClowers commented Apr 26, 2024

I also support option B, I think checking status codes is a good practice anyway. I'm working with this api:

get_region: {
  responses: {
    200: {
      content: {
        "application/json": components["schemas"]["Region"];
      };
    }; 
    202: {
      content: {
        "application/json": components["schemas"]["Region"];
      };
    };
    204: {
      content: never;
    };
  };
};

Currently, data isn't type safe for this API, typescript will let me access properties of Region even though in the 204 case, there is no response body. I misspoke here! It appears that now data is a union of all possible response shapes, which is at least type safe. However, depending on what the shapes in question are, it can be awkward to narrow the type.

Copy link
Contributor

github-actions bot commented Aug 6, 2024

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.

@github-actions github-actions bot added the stale label Aug 6, 2024
Copy link
Contributor

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue! question Further information is requested stale
Projects
None yet
Development

No branches or pull requests

4 participants