feat: narrow type based on status code #1970

DjordyKoert · 2024-10-25T22:56:14Z

Changes

The various client methods (GET, POST, PUT e.t.c.) now have an additional status property.

This property allows users to narrow the type of their data/error based on their documented response status codes.

How to Review

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

Checklist

Unit tests updated
docs/ updated (if necessary)
pnpm run update:examples run (only applicable for openapi-typescript)

changeset-bot · 2024-10-25T22:56:19Z

🦋 Changeset detected

Latest commit: 7aa73ca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
openapi-typescript-helpers	Patch
openapi-react-query	Patch
openapi-fetch	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

drwpow · 2024-11-27T23:15:39Z

I LOVE this! Please let us know if you need any help with the tests or the current direction. This would be amazing to land

packages/openapi-typescript-helpers/index.d.ts

DjordyKoert · 2024-11-28T13:26:28Z

@drwpow I think this is ready for a review now :)

drwpow

This is looking great! But the FIXME in the type tests for if (result.data) vs if (result.error) could be a breaking change, and could be a type error. The status narrowing seems to work brilliantly, but we’ll need to make sure existing users don’t introduce bugs with this new feature by making sure the current type checks work.

If we can find a way to not have the FIXME in the type tests, I think we’ve got it.

.changeset/rare-grapes-explode.md

drwpow · 2024-12-01T21:31:33Z

docs/openapi-fetch/index.md

@@ -158,17 +161,18 @@ The `POST()` request required a `body` object that provided all necessary [reque

 ### Response

-All methods return an object with **data**, **error**, and **response**.
+All methods return an object with **data**, **error**, **status** and **response**.


Great docs updates! Thank you

drwpow · 2024-12-01T21:42:17Z

packages/openapi-fetch/test/common/response.test.ts

@@ -20,22 +20,38 @@ describe("response", () => {
      // 2. assert data is not undefined inside condition block
      if (result.data) {
        assertType<NonNullable<Resource[]>>(result.data);
-        assertType<undefined>(result.error);
+        // @ts-expect-error FIXME: This is a limitation within Typescript


I love these tests, and this is a great start! But I fear that we’ll have to fix this before merging (it’ll be much more difficult to fix in a followup, when other work has built off the inference). I don’t really care if the type is undefined or never—that’s a detail—but TS should be able to infer the other property otherwise there could be issues!

drwpow · 2024-12-01T21:44:08Z

packages/openapi-fetch/test/common/response.test.ts

+
+      if (result.status === 200) {
+        assertType<NonNullable<Resource[]>>(result.data);
+        assertType<never>(result.error);


These are good tests! I would want to see the other common codes tested here, too—201, and 400–405, just for completeness (and if they’re not in the schema, we should add them!)

drwpow · 2024-12-01T21:45:09Z

packages/openapi-fetch/test/common/response.test.ts

+      }
+
+      // @ts-expect-error 204 is not defined in the schema
+      if (result.status === 204) {


This is a REALLY good test I want to keep—testing what happens when a status code is impossible for a given endpoint! But we should test that 204 works somewhere, even if it’s a different test / different endpoint (doesn’t matter where, as long as it’s somewhere, but soft recommendation to just add 204 here since we have all the other checks, and test for the “missing” case in its own test).

gzm0

This is super cool!

I've left some comments mostly regarding http status wildcard precedence.

gzm0 · 2024-12-02T09:08:55Z

packages/openapi-fetch/src/index.d.ts

+    response: Response;
+    status: OpenApiStatusToHttpStatus<S, keyof ResponseObjectMap<T>>;
+    data: S extends OkStatus ? ParseAsResponse<GetResponseContent<ResponseObjectMap<T>, Media, S>, Options> : never;
+    error: S extends ErrorStatus ? GetResponseContent<ResponseObjectMap<T>, Media, S> : never;


This is super nice! I like how it removes "specialness" from data versus error.

IIUC, at this point, the only difference between data and error is parseAs. (totally unrelated to this PR).

I already got a PR semi-ready about that #1986 :)

gzm0 · 2024-12-02T09:16:51Z

packages/openapi-typescript-helpers/index.d.ts

+ * 'default' returns every not explicitly defined status code
+ * '2XX' returns all 2XX status codes
+ * '4XX' returns all 4XX status codes
+ * '5XX' returns all 5XX status codes


Shouldn't we remove the explicitly defined status codes from the wildcards? From the OpenAPI Spec (https://swagger.io/specification/#responses-object)

If a response is defined using an explicit code, the explicit code definition takes precedence over the range definition for that code.

So for example, if we have: "2xx" -> A and "204" -> B, with different, "204" should resolve to B and I think currently it would resolve to A | B.

gzm0 · 2024-12-02T09:23:39Z

packages/openapi-typescript-helpers/index.d.ts

+    ? Exclude<Exclude<OkStatus | ErrorStatus, string>, AllStatuses>
+    : Status extends "2XX"
+      ? Exclude<OkStatus, string>
+      : Status extends "4XX" | "5XX"


It might help readability if you split OkStatus / ErrorStatus into numerics / wildcards. For example:

export type ErrorStatus = NumericErrorStatus | WildcardErrorStatus | "default"; type NumericErrorStatus = 500 | ... type WildcardErrorStatus = '5XX' | '4XX'; export type OpenApiStatusToHttpStatus<Status, AllStatuses> = Status extends number ? Status : Status extends "default" ? Exclude<NumericOKStatus | NumericErrorStatus, AllStatuses> : Status extends WildcardOKStatus ? NumericOKStatus : Status extends WildcardErrorStatus ? NumericErrorStatus : never

(totally optional)

gzm0 · 2024-12-02T09:27:56Z

packages/openapi-fetch/test/types.test.ts

@@ -280,4 +286,179 @@ describe("types", () => {
      assertType<Response>({ error: "default application/json" });
    });
  });
+
+  describe("OpenApiStatusToHttpStatus", () => {


It is super nice to see unit tests for this helper!

I was wondering: IIUC the second argument (AllStatuses) comes directly from the response object keys. In this case, it could contain wildcards (e.g. "2XX"). Would it make sense to add tests for these as well? (I have tried pointing out places below where I think this might affect the tests).

@drwpow self note to maintainers: we should probably set up the infrastructure for openapi-typescript-helpers to have tests :)

gzm0 · 2024-12-02T09:34:01Z

packages/openapi-fetch/test/types.test.ts

+    });
+
+    test("returns 200 likes response", () => {
+      type Status = OpenApiStatusToHttpStatus<"2XX", 200 | 204 | 206 | 404 | 500 | "default">;


It feels to me like this test case is "degenerate" at least if I understand the usage in this PR correctly:

I would expect the Status parameter to always be a subtype of AllStatuses (which in this case it isn't).

For this test case specifically, I would expect that the explicitly / numerically defined status codes are not part of the end result (precedence, as I pointed out in the doc comment of OpenApiStatusToHttpStatus).

gzm0 · 2024-12-02T09:37:04Z

packages/openapi-fetch/test/types.test.ts

+    });
+
+    test("returns default response", () => {
+      type Status = OpenApiStatusToHttpStatus<"default", 200 | 204 | 206 | 404 | 500 | "default">;


Would it make sense add a test case where there also is a wildcard (e.g. "2XX") in the second argument?

gzm0 · 2024-12-02T09:47:09Z

packages/openapi-typescript-helpers/index.d.ts

+ * '2XX' returns all 2XX status codes
+ * '4XX' returns all 4XX status codes
+ * '5XX' returns all 5XX status codes
+ */


Note: IIUC this implementation has the limitation that status codes unknown to openapi-fetch have to be explicitly defined in the OpenAPI spec so they can be typed on the client side.

For example, "2XX" in the OpenAPI spec will not match 209:

if (status === 209) { // `data` will be never }

IMO this is a reasonable limitation / design decision.

Relevant excerpt from RFC 7231

HTTP status codes are extensible. HTTP clients are not required to
understand the meaning of all registered status codes, though such
understanding is obviously desirable. However, a client MUST
understand the class of any status code, as indicated by the first
digit, and treat an unrecognized status code as being equivalent to
the x00 status code of that class, with the exception that a
recipient MUST NOT cache a response with an unrecognized status code.

For example, if an unrecognized status code of 471 is received by a
client, the client can assume that there was something wrong with its
request and treat the response as if it had received a 400 (Bad
Request) status code. The response message will usually contain a
representation that explains the status.

Co-authored-by: Drew Powers <[email protected]>

DjordyKoert added 6 commits October 25, 2024 23:56

remove invisible character

5e5b7fd

return status from fetch

de4b652

type narrowing from status

7ce026e

fallback to undefined | never

af40b61

add test for type narrowing

650c88b

lint-fix

6c405f8

drwpow added the openapi-fetch Relevant to the openapi-fetch library label Oct 26, 2024

DjordyKoert added 8 commits November 28, 2024 11:42

Ensure default response type gets parsed properly

aea776a

Remove Required

17dd38a

add status type test

9e4e0a0

simplify type

da41303

remove unused imports

c11622d

cleanup GetResponseContent

a969c13

fix: GetResponseContent not being exported

a3422b2

do not use ParseAsResponse for error

c4f66ce

DjordyKoert force-pushed the type-narrow-on-status-code branch from ff0685c to c4f66ce Compare November 28, 2024 12:49

DjordyKoert commented Nov 28, 2024

View reviewed changes

packages/openapi-typescript-helpers/index.d.ts Show resolved Hide resolved

DjordyKoert added 2 commits November 28, 2024 13:54

changeset

8fcfc57

add status docs

21c7f26

DjordyKoert marked this pull request as ready for review November 28, 2024 13:26

DjordyKoert requested a review from a team as a code owner November 28, 2024 13:26

DjordyKoert requested a review from gzm0 November 28, 2024 13:26

DjordyKoert force-pushed the type-narrow-on-status-code branch from 7cff619 to 21c7f26 Compare November 28, 2024 13:32

update status explanation with link to mozilla documentation

4f8192a

drwpow requested changes Dec 1, 2024

View reviewed changes

gzm0 reviewed Dec 2, 2024

View reviewed changes

Change version bump to patch

7aa73ca

Co-authored-by: Drew Powers <[email protected]>

gzm0 mentioned this pull request Dec 9, 2024

fix(#1977): refactor handling for empty responses #1986

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: narrow type based on status code #1970

feat: narrow type based on status code #1970

DjordyKoert commented Oct 25, 2024 •

edited

Loading

changeset-bot bot commented Oct 25, 2024 •

edited

Loading

drwpow commented Nov 27, 2024

DjordyKoert commented Nov 28, 2024

drwpow left a comment •

edited

Loading

drwpow Dec 1, 2024

drwpow Dec 1, 2024

drwpow Dec 1, 2024

drwpow Dec 1, 2024

gzm0 left a comment

gzm0 Dec 2, 2024

DjordyKoert Dec 6, 2024

gzm0 Dec 2, 2024

gzm0 Dec 2, 2024

gzm0 Dec 2, 2024

gzm0 Dec 2, 2024

gzm0 Dec 2, 2024

gzm0 Dec 2, 2024

feat: narrow type based on status code #1970

Are you sure you want to change the base?

feat: narrow type based on status code #1970

Conversation

DjordyKoert commented Oct 25, 2024 • edited Loading

Changes

How to Review

Checklist

changeset-bot bot commented Oct 25, 2024 • edited Loading

🦋 Changeset detected

drwpow commented Nov 27, 2024

DjordyKoert commented Nov 28, 2024

drwpow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DjordyKoert commented Oct 25, 2024 •

edited

Loading

changeset-bot bot commented Oct 25, 2024 •

edited

Loading

drwpow left a comment •

edited

Loading