Skip to content

feat: narrow type based on status code #1970

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.

Sign up for GitHub

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

feat: narrow type based on status code #1970

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5e5b7fd
remove invisible character
DjordyKoert Oct 25, 2024
de4b652
return status from fetch
DjordyKoert Oct 25, 2024
7ce026e
type narrowing from status
DjordyKoert Oct 25, 2024
af40b61
fallback to undefined | never
DjordyKoert Oct 25, 2024
650c88b
add test for type narrowing
DjordyKoert Oct 25, 2024
6c405f8
lint-fix
DjordyKoert Oct 25, 2024
aea776a
Ensure default response type gets parsed properly
DjordyKoert Nov 5, 2024
17dd38a
Remove Required
DjordyKoert Nov 28, 2024
9e4e0a0
add status type test
DjordyKoert Nov 28, 2024
da41303
simplify type
DjordyKoert Nov 28, 2024
c11622d
remove unused imports
DjordyKoert Nov 28, 2024
a969c13
cleanup GetResponseContent
DjordyKoert Nov 28, 2024
a3422b2
fix: GetResponseContent not being exported
DjordyKoert Nov 28, 2024
c4f66ce
do not use ParseAsResponse for error
DjordyKoert Nov 28, 2024
8fcfc57
changeset
DjordyKoert Nov 28, 2024
21c7f26
add status docs
DjordyKoert Nov 28, 2024
4f8192a
update `status` explanation with link to mozilla documentation
DjordyKoert Nov 28, 2024
7aa73ca
Change version bump to patch
DjordyKoert Dec 6, 2024
30031bb
Merge branch 'main' into type-narrow-on-status-code
drwpow Jan 25, 2025
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
25 changes: 12 additions & 13 deletions packages/openapi-fetch/src/index.d.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 400–405, 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
35 changes: 35 additions & 0 deletions packages/openapi-fetch/test/never-response/never-response.test.ts
View file
Open in desktop
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);
}
});
});
183 changes: 182 additions & 1 deletion packages/openapi-fetch/test/types.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { assertType, describe, test } from "vitest";
import type { ErrorResponse, GetResponseContent, OkStatus, SuccessResponse } from "openapi-typescript-helpers";
import type {
ErrorResponse,
GetResponseContent,
OkStatus,
OpenApiStatusToHttpStatus,
SuccessResponse,
} from "openapi-typescript-helpers";

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

describe("OpenApiStatusToHttpStatus", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

test("returns numeric status code", () => {
assertType<OpenApiStatusToHttpStatus<200, number>>(200);
assertType<OpenApiStatusToHttpStatus<200, string>>(200);
assertType<OpenApiStatusToHttpStatus<204, string>>(204);

assertType<OpenApiStatusToHttpStatus<204, string>>(
// @ts-expect-error 200 is not a valid
200,
);
assertType<OpenApiStatusToHttpStatus<204, number>>(
// @ts-expect-error 200 is not a valid
200,
);

assertType<OpenApiStatusToHttpStatus<404, 200 | 204 | 206 | 404 | 500 | "default">>(404);
});

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

Choose a reason for hiding this comment

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

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

assertType<Status>(
// @ts-expect-error 200 has been manually defined
200,
);
assertType<Status>(
// @ts-expect-error 204 has been manually defined
204,
);
assertType<Status>(201);
assertType<Status>(504);
});

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

Choose a reason for hiding this comment

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

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

assertType<Status>(200);
assertType<Status>(201);
assertType<Status>(202);
assertType<Status>(203);
assertType<Status>(204);
assertType<Status>(
// @ts-expect-error 205 is not a valid 2XX status code
205,
);
assertType<Status>(206);
assertType<Status>(207);
assertType<Status>(
// @ts-expect-error '2XX' is not a numeric status code
"2XX",
);
assertType<Status>(
// @ts-expect-error 205 is not a valid 2XX status code
208,
);

assertType<Status>(
// @ts-expect-error '4XX' is not a numeric status code
"4XX",
);
assertType<Status>(
// @ts-expect-error '5XX' is not a numeric status code
"5XX",
);
});

test("returns error responses for 4XX", () => {
type Status = OpenApiStatusToHttpStatus<"4XX", 200 | 204 | 206 | 404 | 500 | "default">;
assertType<Status>(400);
assertType<Status>(401);
assertType<Status>(402);
assertType<Status>(403);
assertType<Status>(404);
assertType<Status>(405);
assertType<Status>(406);
assertType<Status>(407);
assertType<Status>(408);
assertType<Status>(409);
assertType<Status>(410);
assertType<Status>(411);
assertType<Status>(412);
assertType<Status>(413);
assertType<Status>(414);
assertType<Status>(415);
assertType<Status>(416);
assertType<Status>(417);
assertType<Status>(418);
assertType<Status>(500);
assertType<Status>(501);
assertType<Status>(502);
assertType<Status>(503);
assertType<Status>(504);
assertType<Status>(505);
assertType<Status>(506);
assertType<Status>(507);
assertType<Status>(508);
assertType<Status>(
// @ts-expect-error 509 is not a valid error status code
509,
);
assertType<Status>(510);
assertType<Status>(511);

assertType<Status>(
// @ts-expect-error 200 is not a valid error status code
200,
);
assertType<Status>(
// @ts-expect-error '2XX' is not a numeric status code
"2XX",
);
assertType<Status>(
// @ts-expect-error '4XX' is not a numeric status code
"4XX",
);
assertType<Status>(
// @ts-expect-error '5XX' is not a numeric status code
"5XX",
);
});

test("returns error responses for 5XX", () => {
type Status = OpenApiStatusToHttpStatus<"5XX", 200 | 204 | 206 | 404 | 500 | "default">;
assertType<Status>(400);
assertType<Status>(401);
assertType<Status>(402);
assertType<Status>(403);
assertType<Status>(404);
assertType<Status>(405);
assertType<Status>(406);
assertType<Status>(407);
assertType<Status>(408);
assertType<Status>(409);
assertType<Status>(410);
assertType<Status>(411);
assertType<Status>(412);
assertType<Status>(413);
assertType<Status>(414);
assertType<Status>(415);
assertType<Status>(416);
assertType<Status>(417);
assertType<Status>(418);
assertType<Status>(500);
assertType<Status>(501);
assertType<Status>(502);
assertType<Status>(503);
assertType<Status>(504);
assertType<Status>(505);
assertType<Status>(506);
assertType<Status>(507);
assertType<Status>(508);
assertType<Status>(
// @ts-expect-error 509 is not a valid error status code
509,
);
assertType<Status>(510);
assertType<Status>(511);

assertType<Status>(
// @ts-expect-error 200 is not a valid error status code
200,
);
assertType<Status>(
// @ts-expect-error '2XX' is not a numeric status code
"2XX",
);
assertType<Status>(
// @ts-expect-error '4XX' is not a numeric status code
"4XX",
);
assertType<Status>(
// @ts-expect-error '5XX' is not a numeric status code
"5XX",
);
});
});
});
Loading