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

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

Merged
merged 12 commits into from
Oct 25, 2024
6 changes: 6 additions & 0 deletions .changeset/small-jokes-wait.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"openapi-typescript-helpers": patch
"openapi-fetch": patch
---

client data & error now return a union of possible types
6 changes: 3 additions & 3 deletions packages/openapi-fetch/src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ 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> =
export type FetchResponse<T extends Record<string | number, any>, Options, Media extends MediaType> =
| {
data: ParseAsResponse<SuccessResponse<ResponseObjectMap<T>, Media>, Options>;
error?: never;
Expand Down Expand Up @@ -187,7 +187,7 @@ export type ClientMethod<
...init: InitParam<Init>
) => Promise<FetchResponse<Paths[Path][Method], Init, Media>>;

export type ClientForPath<PathInfo, Media extends MediaType> = {
export type ClientForPath<PathInfo extends Record<string | number, any>, Media extends MediaType> = {
[Method in keyof PathInfo as Uppercase<string & Method>]: <Init extends MaybeOptionalInit<PathInfo, Method>>(
...init: InitParam<Init>
) => Promise<FetchResponse<PathInfo[Method], Init, Media>>;
Expand Down Expand Up @@ -234,7 +234,7 @@ export default function createClient<Paths extends {}, Media extends MediaType =
clientOptions?: ClientOptions,
): Client<Paths, Media>;

export type PathBasedClient<Paths, Media extends MediaType = MediaType> = {
export type PathBasedClient<Paths extends Record<string | number, any>, Media extends MediaType = MediaType> = {
[Path in keyof Paths]: ClientForPath<Paths[Path], Media>;
};

Expand Down
6 changes: 3 additions & 3 deletions packages/openapi-fetch/test/common/response.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ describe("response", () => {
{},
);

assertType<undefined>(result.data);
// @ts-expect-error: FIXME when #1723 is resolved; this shouldn’t throw an error
//@ts-expect-error impossible to determine data type for invalid path
assertType<never>(result.data);
DjordyKoert marked this conversation as resolved.
Show resolved Hide resolved
assertType<undefined>(result.error);
});

Expand All @@ -74,7 +74,7 @@ describe("response", () => {
} else {
expectTypeOf(result.data).toBeUndefined();
expectTypeOf(result.error).extract<{ code: number }>().toEqualTypeOf<{ code: number; message: string }>();
expectTypeOf(result.error).exclude<{ code: number }>().toEqualTypeOf<never>();
expectTypeOf(result.error).exclude<{ code: number }>().toEqualTypeOf(undefined);
}
});

Expand Down
143 changes: 143 additions & 0 deletions packages/openapi-fetch/test/never-response/never-response.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
import { assertType, describe, expect, test } from "vitest";
import { createObservedClient } from "../helpers.js";
import type { components, paths } from "./schemas/never-response.js";

describe("GET", () => {
test("sends correct method", async () => {
let method = "";
const client = createObservedClient<paths>({}, async (req) => {
method = req.method;
return Response.json({});
});
await client.GET("/posts");
expect(method).toBe("GET");
});

test("sends correct options, returns success", 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, response } = await client.GET("/posts/{id}", {
params: { path: { id: 123 } },
});

assertType<typeof mockData | undefined>(data);

// assert correct URL was called
expect(actualPathname).toBe("/posts/123");

// assert correct data was returned
expect(data).toEqual(mockData);
expect(response.status).toBe(200);

// assert error is empty
expect(error).toBeUndefined();
});

test("sends correct options, returns undefined on 204", async () => {
let actualPathname = "";
const client = createObservedClient<paths>({}, async (req) => {
actualPathname = new URL(req.url).pathname;
return new Response(null, { status: 204 });
});

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

assertType<components["schemas"]["Post"] | undefined>(data);

// assert correct URL was called
expect(actualPathname).toBe("/posts/123");

// assert 204 to be transformed to empty object
expect(data).toEqual({});
Comment on lines +61 to +62
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

  1. This makes it impossible to have proper types for 204 codes.
  2. 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 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Keep this PR as-is, release as a patch, then release that 204 change as a minor (breaking) change, OR
  2. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😎

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

expect(response.status).toBe(204);

// assert error is empty
expect(error).toBeUndefined();
});

test("sends correct options, returns error", async () => {
const mockError = { code: 404, message: "Post not found" };

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

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

assertType<typeof mockError | undefined>(error);

// assert correct URL was called
expect(actualPathname).toBe("/posts/123");

// assert correct method was called
expect(method).toBe("GET");

// assert correct error was returned
expect(error).toEqual(mockError);
expect(response.status).toBe(404);

// assert data is empty
expect(data).toBeUndefined();
});

test("handles array-type responses", async () => {
const client = createObservedClient<paths>({}, async () => Response.json([]));

const { data } = await client.GET("/posts", { params: {} });
if (!data) {
throw new Error("data empty");
}

// assert array type (and only array type) was inferred
expect(data.length).toBe(0);
});

test("handles empty-array-type 204 response", async () => {
let method = "";
let actualPathname = "";
const client = createObservedClient<paths>({}, async (req) => {
method = req.method;
actualPathname = new URL(req.url).pathname;
return new Response(null, { status: 204 });
});

const { data } = await client.GET("/posts", { params: {} });

assertType<components["schemas"]["Post"][] | unknown[] | undefined>(data);

// assert correct URL was called
expect(actualPathname).toBe("/posts");

// assert correct method was called
expect(method).toBe("GET");

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

test("gracefully handles invalid JSON for errors", async () => {
const client = createObservedClient<paths>({}, async () => new Response("Unauthorized", { status: 401 }));

const { data, error } = await client.GET("/posts");

expect(data).toBeUndefined();
expect(error).toBe("Unauthorized");
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
/**
* This file was auto-generated by openapi-typescript.
* Do not make direct changes to the file.
*/

export interface paths {
"/posts": {
parameters: {
query?: never;
header?: never;
path?: never;
cookie?: never;
};
get: {
parameters: {
query?: never;
header?: never;
path?: never;
cookie?: never;
};
requestBody?: never;
responses: {
/** @description OK */
200: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": components["schemas"]["Post"][];
};
};
/** @description No posts found, but it's OK */
204: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": unknown[];
};
};
/** @description Unexpected error */
default: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": components["schemas"]["Error"];
};
};
};
};
put?: never;
post?: never;
delete?: never;
options?: never;
head?: never;
patch?: never;
trace?: never;
};
"/posts/{id}": {
parameters: {
query?: never;
header?: never;
path: {
id: number;
};
cookie?: never;
};
get: {
parameters: {
query?: never;
header?: never;
path: {
id: number;
};
cookie?: never;
};
requestBody?: never;
responses: {
/** @description OK */
200: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": components["schemas"]["Post"];
};
};
/** @description No post found, but it's OK */
204: {
headers: {
[name: string]: unknown;
};
content?: never;
};
/** @description A weird error happened */
500: {
headers: {
[name: string]: unknown;
};
content?: never;
};
/** @description Unexpected error */
default: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": components["schemas"]["Error"];
};
};
};
};
put?: never;
post?: never;
delete?: never;
options?: never;
head?: never;
patch?: never;
trace?: never;
};
}
export type webhooks = Record<string, never>;
export interface components {
schemas: {
Error: {
code: number;
message: string;
};
Post: {
id: number;
title: string;
};
};
responses: never;
parameters: never;
requestBodies: never;
headers: never;
pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;
Loading