fix(openapi-fetch): Return union of possible responses (data & error) #1937

DjordyKoert · 2024-10-03T08:31:31Z

Changes

fixes #1723, fixes #1933, fixes #1820

This change ensures that a union of possible types from the response content is returned.

How to Review

The behaviour of the new type can be seen in the newly added types.test.ts file. This also helps ensure possible breaking changes in the future are caught by the failing test(s) (these types are exported after all and can be used by users of this package).

Checklist

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

changeset-bot · 2024-10-03T08:31:34Z

🦋 Changeset detected

Latest commit: 080673d

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-fetch	Patch
openapi-react-query	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

DjordyKoert · 2024-10-03T09:20:09Z

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

+    // assert 204 to be transformed to empty object
+    expect(data).toEqual({});


Side-note:

I do not fully agree with this behaviour at https://github.com/openapi-ts/openapi-typescript/blob/main/packages/openapi-fetch/src/index.js#L155.

This makes it impossible to have proper types for 204 codes.

If users of this package DO return some data from 204's (this shouldn't be the case of course, but if their openapi docs DO specify this then it should still return the data from the response instead of transforming it like this)

This PR ensures that a documented 204 response (content?: never) will return a data as a possible undefined. BUT this will now never be the case because of this behaviour.

Would love to hear some feedback and if it could be considered to remove this 😄

Yes we’re open to revisiting that behavior! Agree that at this point, that does more harm than good. That was an early behavior that just never received much questioning until now.

I’d be open to either of the following:

Keep this PR as-is, release as a patch, then release that 204 change as a minor (breaking) change, OR

Include that change in this PR, and release everything as a minor (breaking) change

Either way it would constitute a breaking change, but IMO a welcome one. No strong preference on #1 or #2, but #1 would be easier to release faster

My preference would also be to keep this PR as-is aswell. I will make a new PR to fix the 204 thing soon 😄

After that I will have a look at what @zkulbeda suggested for exposing the status to help narrowing the data type even more 😎

That works! Also noticed there’s a tiny conflict with the openapi-typescript-helpers; can merge once that’s resolved.

Also feel free to make an adjustment there if it causes a regression; that recent change was just an arbitrary QoL improvement and didn’t fix any type issues like your PR does

Great! Fixed the feedback you gave so everything should be good now 😄

zkulbeda · 2024-10-12T20:16:19Z

Hello! I have an idea how to improve DX for function's return union type using type narrowing. By refining the FetchResponse type to account for the specific HTTP status codes, you could make it much easier for developers to handle responses safely and intuitively.

const { status, data, error } = await client.GET(pathname);

if (status === 200) {
 // TypeScript narrows 'status' to 200, so:
 // - 'data' is the type associated with status 200
 // - This is more specific than the general 'data' type for all 2xx responses
 data; // Autocompletion shows properties specific to 200 responses
}

Rough draft of code implementation:

diff --git a/packages/openapi-fetch/src/index.d.ts b/packages/openapi-fetch/src/index.d.ts
index 4941612..64395b9 100644
--- a/packages/openapi-fetch/src/index.d.ts
+++ b/packages/openapi-fetch/src/index.d.ts
@@ -1,5 +1,5 @@
 import type {
-  ErrorResponse,
+  FetchResult,
   FilterKeys,
   HttpMethod,
   IsOperationRequestBodyOptional,
@@ -8,7 +8,6 @@ import type {
   PathsWithMethod,
   ResponseObjectMap,
   RequiredKeysOf,
-  SuccessResponse,
 } from "openapi-typescript-helpers";
 
 /** Options for each client instance */
@@ -98,17 +97,12 @@ export type RequestBodyOption<T> = OperationRequestBodyContent<T> extends never
 
 export type FetchOptions<T> = RequestOptions<T> & Omit<RequestInit, "body" | "headers">;
 
-export type FetchResponse<T, Options, Media extends MediaType> =
-  | {
-      data: ParseAsResponse<SuccessResponse<ResponseObjectMap<T>, Media>, Options>;
-      error?: never;
-      response: Response;
-    }
-  | {
-      data?: never;
-      error: ErrorResponse<ResponseObjectMap<T>, Media>;
-      response: Response;
-    };
+export type FetchResponse<T, Options, Media extends MediaType> = {
+  response: Response;
+  error?: never;
+  data?: never;
+  status: number;
+} & ParseAsResponse<FetchResult<ResponseObjectMap<T>, Media>, Options>;
 
 export type RequestOptions<T> = ParamsOption<T> &
   RequestBodyOption<T> & {
diff --git a/packages/openapi-typescript-helpers/index.d.ts b/packages/openapi-typescript-helpers/index.d.ts
index d8e67c5..f8589c4 100644
--- a/packages/openapi-typescript-helpers/index.d.ts
+++ b/packages/openapi-typescript-helpers/index.d.ts
@@ -122,6 +122,32 @@ export type SuccessResponse<T, Media extends MediaType = MediaType> = FilterKeys
   Media
 >;
 
+export type FetchResult<T, Media extends MediaType = MediaType> = {
+  [K in keyof T]: K extends "2XX"
+    ? {
+        status: number;
+        data: FilterKeys<ResponseContent<T[K]>, Media>;
+        error?: never;
+      }
+    : K extends "4XX" | "5XX" | "default"
+      ? {
+          status: number;
+          data?: never;
+          error: FilterKeys<ResponseContent<T[K]>, Media>;
+        }
+      : K extends OkStatus
+        ? {
+            status: K;
+            data: FilterKeys<ResponseContent<T[K]>, Media>;
+            error?: never;
+          }
+        : {
+            status: K;
+            data?: never;
+            error: FilterKeys<ResponseContent<T[K]>, Media>;
+          };
+}[keyof T];
+
 /**
  * Return first 5XX or 4XX response (in that order) from a Response Object Map
  */

drwpow

This is a FANTASTIC PR, thank you! Great tests that ensure it’s working.

Will wait to merge/release depending on whether or not you’d like to tackle the 204 breaking behavior change as part of this PR, or in a followup. Either works.

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

drwpow · 2024-10-25T16:20:46Z

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

+    // assert 204 to be transformed to empty object
+    expect(data).toEqual({});


Yes we’re open to revisiting that behavior! Agree that at this point, that does more harm than good. That was an early behavior that just never received much questioning until now.

I’d be open to either of the following:

Keep this PR as-is, release as a patch, then release that 204 change as a minor (breaking) change, OR

Include that change in this PR, and release everything as a minor (breaking) change

Either way it would constitute a breaking change, but IMO a welcome one. No strong preference on #1 or #2, but #1 would be easier to release faster

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

# Conflicts: # packages/openapi-typescript-helpers/index.d.ts

drwpow

Awesome work. Thanks again! 🙂 Will release this now.

DjordyKoert added 4 commits October 2, 2024 17:45

Get all possible responses as a union

f8df0e1

Add tests

4b4fb7a

lint fix

6e354a8

Add tests for standalone types

ed3edeb

DjordyKoert requested a review from a team as a code owner October 3, 2024 08:31

lint fix

0c915ba

DjordyKoert marked this pull request as draft October 3, 2024 08:36

DjordyKoert added 4 commits October 3, 2024 10:39

changeset

55da875

Fix passing more keys (OkStatus) returning unknown

b9501b7

add test for non-existent media type

ef577d1

Update invalid path test

62f0329

DjordyKoert commented Oct 3, 2024

View reviewed changes

DjordyKoert marked this pull request as ready for review October 3, 2024 09:20

kerwanp added openapi-ts Relevant to the openapi-typescript library openapi-fetch Relevant to the openapi-fetch library and removed openapi-ts Relevant to the openapi-typescript library labels Oct 14, 2024

drwpow approved these changes Oct 25, 2024

View reviewed changes

DjordyKoert added 3 commits October 25, 2024 19:09

Merge branch 'main' into fix-1933

cb027ec

# Conflicts: # packages/openapi-typescript-helpers/index.d.ts

lint-fix

d307e52

better @ts-expect-error scoping

080673d

DjordyKoert requested a review from drwpow October 25, 2024 17:28

drwpow approved these changes Oct 25, 2024

View reviewed changes

drwpow merged commit 06163a2 into openapi-ts:main Oct 25, 2024
8 checks passed

openapi-ts-bot mentioned this pull request Oct 25, 2024

[ci] release #1965

Merged

DjordyKoert mentioned this pull request Oct 25, 2024

feat: 204 to undefined #1968

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(openapi-fetch): Return union of possible responses (data & error) #1937

fix(openapi-fetch): Return union of possible responses (data & error) #1937

DjordyKoert commented Oct 3, 2024 •

edited

Loading

changeset-bot bot commented Oct 3, 2024 •

edited

Loading

DjordyKoert Oct 3, 2024

drwpow Oct 25, 2024

DjordyKoert Oct 25, 2024

drwpow Oct 25, 2024

DjordyKoert Oct 25, 2024

zkulbeda commented Oct 12, 2024 •

edited

Loading

drwpow left a comment

drwpow Oct 25, 2024

drwpow left a comment

		// assert 204 to be transformed to empty object
		expect(data).toEqual({});

fix(openapi-fetch): Return union of possible responses (data & error) #1937

fix(openapi-fetch): Return union of possible responses (data & error) #1937

Conversation

DjordyKoert commented Oct 3, 2024 • edited Loading

Changes

How to Review

Checklist

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

🦋 Changeset detected

DjordyKoert Oct 3, 2024

Choose a reason for hiding this comment

drwpow Oct 25, 2024

Choose a reason for hiding this comment

DjordyKoert Oct 25, 2024

Choose a reason for hiding this comment

drwpow Oct 25, 2024

Choose a reason for hiding this comment

DjordyKoert Oct 25, 2024

Choose a reason for hiding this comment

zkulbeda commented Oct 12, 2024 • edited Loading

drwpow left a comment

Choose a reason for hiding this comment

drwpow Oct 25, 2024

Choose a reason for hiding this comment

drwpow left a comment

Choose a reason for hiding this comment

DjordyKoert commented Oct 3, 2024 •

edited

Loading

changeset-bot bot commented Oct 3, 2024 •

edited

Loading

zkulbeda commented Oct 12, 2024 •

edited

Loading