Skip to content
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

feat: narrow type based on status code #1970

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/rare-grapes-explode.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"openapi-typescript-helpers": minor
DjordyKoert marked this conversation as resolved.
Show resolved Hide resolved
"openapi-react-query": minor
DjordyKoert marked this conversation as resolved.
Show resolved Hide resolved
"openapi-fetch": minor
DjordyKoert marked this conversation as resolved.
Show resolved Hide resolved
---

add `status` to openapi-fetch & type narrowing for `data` and `error` based on `status`
18 changes: 11 additions & 7 deletions docs/openapi-fetch/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const client = createClient<paths>({ baseUrl: "https://myapi.dev/v1/" });
const {
data, // only present if 2XX response
error, // only present if 4XX or 5XX response
status, // HTTP Status code
} = await client.GET("/blogposts/{post_id}", {
params: {
path: { post_id: "123" },
Expand All @@ -47,6 +48,8 @@ await client.PUT("/blogposts", {

`data` and `error` are typechecked and expose their shapes to Intellisense in VS Code (and any other IDE with TypeScript support). Likewise, the request `body` will also typecheck its fields, erring if any required params are missing, or if there’s a type mismatch.

The `status` property is also available and contains the HTTP status code of the response (`response.status`). This property is useful for handling different status codes in your application and narrowing down the `data` and `error` properties.

`GET()`, `PUT()`, `POST()`, etc. are thin wrappers around the native [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) (which you can [swap for any call](/openapi-fetch/api#create-client)).

Notice there are no generics, and no manual typing. Your endpoint’s request and response were inferred automatically. This is a huge improvement in the type safety of your endpoints because **every manual assertion could lead to a bug**! This eliminates all of the following:
Expand Down Expand Up @@ -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**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great docs updates! Thank you


```ts
const { data, error, response } = await client.GET("/url");
const { data, error, status, response } = await client.GET("/url");
```

| Object | Response |
| :--------- | :-------------------------------------------------------------------------------------------------------------------------- |
| `data` | `2xx` response if OK; otherwise `undefined` |
| `error` | `5xx`, `4xx`, or `default` response if not OK; otherwise `undefined` |
| `response` | [The original Response](https://developer.mozilla.org/en-US/docs/Web/API/Response) which contains `status`, `headers`, etc. |
| Object | Response |
|:-----------|:------------------------------------------------------------------------------------------------------------------------------|
| `data` | `2xx` response if OK; otherwise `undefined` |
| `error` | `5xx`, `4xx`, or `default` response if not OK; otherwise `undefined` |
| `status` | The HTTP response status code of [the original response](https://developer.mozilla.org/en-US/docs/Web/API/Response/status) |
| `response` | [The original Response](https://developer.mozilla.org/en-US/docs/Web/API/Response) which contains `status`, `headers`, etc. |

### Path-property style

Expand Down
25 changes: 12 additions & 13 deletions packages/openapi-fetch/src/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type {
ErrorResponse,
FilterKeys,
HttpMethod,
IsOperationRequestBodyOptional,
Expand All @@ -8,7 +7,10 @@ import type {
PathsWithMethod,
ResponseObjectMap,
RequiredKeysOf,
SuccessResponse,
GetResponseContent,
ErrorStatus,
OkStatus,
OpenApiStatusToHttpStatus,
} from "openapi-typescript-helpers";

/** Options for each client instance */
Expand Down Expand Up @@ -98,17 +100,14 @@ export type RequestBodyOption<T> = OperationRequestBodyContent<T> extends never

export type FetchOptions<T> = RequestOptions<T> & Omit<RequestInit, "body" | "headers">;

export type FetchResponse<T extends Record<string | number, any>, 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 extends Record<string | number, any>, Options, Media extends MediaType> = {
[S in keyof ResponseObjectMap<T>]: {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};
}[keyof ResponseObjectMap<T>];

export type RequestOptions<T> = ParamsOption<T> &
RequestBodyOption<T> & {
Expand Down
10 changes: 6 additions & 4 deletions packages/openapi-fetch/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,18 @@ export default function createClient(clientOptions) {

// handle empty content
if (response.status === 204 || response.headers.get("Content-Length") === "0") {
return response.ok ? { data: undefined, response } : { error: undefined, response };
return response.ok
? { data: undefined, response, status: response.status }
: { error: undefined, response, status: response.status };
}

// parse response (falling back to .text() when necessary)
if (response.ok) {
// if "stream", skip parsing entirely
if (parseAs === "stream") {
return { data: response.body, response };
return { data: response.body, response, status: response.status };
}
return { data: await response[parseAs](), response };
return { data: await response[parseAs](), response, status: response.status };
}

// handle errors
Expand All @@ -171,7 +173,7 @@ export default function createClient(clientOptions) {
} catch {
// noop
}
return { error, response };
return { error, response, status: response.status };
}

return {
Expand Down
31 changes: 23 additions & 8 deletions packages/openapi-fetch/test/common/response.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

assertType<never>(result.error);
}
// 2b. inverse should work, too
if (!result.error) {
assertType<NonNullable<Resource[]>>(result.data);
assertType<undefined>(result.error);
assertType<never>(result.error);
}

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

Choose a reason for hiding this comment

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

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

}

if (result.status === 500) {
assertType<never>(result.data);
assertType<Error>(result.error);
}

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

Choose a reason for hiding this comment

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

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).

}

// 3. assert error is not undefined inside condition block
if (result.error) {
assertType<undefined>(result.data);
// @ts-expect-error FIXME: This is a limitation within Typescript
assertType<never>(result.data);
assertType<NonNullable<Error>>(result.error);
}
// 3b. inverse should work, too
if (!result.data) {
assertType<undefined>(result.data);
assertType<never>(result.data);
assertType<NonNullable<Error>>(result.error);
}
});
Expand All @@ -49,9 +65,8 @@ describe("response", () => {
{},
);

//@ts-expect-error impossible to determine data type for invalid path
assertType<never>(result.data);
assertType<undefined>(result.error);
assertType<never>(result.error);
});

test("returns union for mismatched response", async () => {
Expand All @@ -65,14 +80,14 @@ describe("response", () => {
}
});

test("returns union for mismatched errors", async () => {
test("returns union for mismatched errors", async () => {
const client = createObservedClient<paths>();
const result = await client.GET("/mismatched-errors");
if (result.data) {
expectTypeOf(result.data).toEqualTypeOf<Resource>();
expectTypeOf(result.data).toEqualTypeOf<MethodResponse<typeof client, "get", "/mismatched-errors">>();
} else {
expectTypeOf(result.data).toBeUndefined();
expectTypeOf(result.data).toBeNever();
expectTypeOf(result.error).extract<{ code: number }>().toEqualTypeOf<{ code: number; message: string }>();
expectTypeOf(result.error).exclude<{ code: number }>().toEqualTypeOf(undefined);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,4 +140,39 @@ describe("GET", () => {
expect(data).toBeUndefined();
expect(error).toBe("Unauthorized");
});

test("type narrowing on status", async () => {
const mockData = {
id: 123,
title: "My Post",
};

let actualPathname = "";
const client = createObservedClient<paths>({}, async (req) => {
actualPathname = new URL(req.url).pathname;
return Response.json(mockData);
});

const { data, error, status } = await client.GET("/posts/{id}", {
params: { path: { id: 123 } },
});

if (status === 200) {
assertType<typeof mockData>(data);
assertType<never>(error);
} else if (status === 204) {
assertType<undefined>(data);
} else if (status === 400) {
assertType<components["schemas"]["Error"]>(error);
} else if (status === 201) {
// Grabs the 'default' response
assertType<components["schemas"]["Error"]>(error);
} else if (status === 500) {
assertType<never>(data);
assertType<undefined>(error);
} else {
// All other status codes are handles with the 'default' response
assertType<components["schemas"]["Error"]>(error);
}
});
});
Loading