Skip to content

Fix empty object required 2nd param #1366

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

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Fix empty object required 2nd param #1366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions .changeset/honest-yaks-smell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"openapi-fetch": patch
---

Fix empty object being required param
5 changes: 5 additions & 0 deletions .changeset/silent-carpets-grab.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"openapi-typescript-helpers": patch
---

Add HasRequiredKeys<T> helper
36 changes: 18 additions & 18 deletions packages/openapi-fetch/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe("client", () => {

// data
mockFetchOnce({ status: 200, body: JSON.stringify(["one", "two", "three"]) });
const dataRes = await client.GET("/string-array", {});
const dataRes = await client.GET("/string-array");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: the removal of all , {}) in the test suite is a good sign. The test suite has quite a few // @ts-expect-error comments which are vital to testing the type assertions.

But in addition, the test suite also has runtime tests to ensure the function is behaving as-expected. The fact that no tests were modified, while still passing, is a great sign.


// … is initially possibly undefined
// @ts-expect-error
Expand All @@ -67,7 +67,7 @@ describe("client", () => {

// error
mockFetchOnce({ status: 500, body: JSON.stringify({ code: 500, message: "Something went wrong" }) });
const errorRes = await client.GET("/string-array", {});
const errorRes = await client.GET("/string-array");

// … is initially possibly undefined
// @ts-expect-error
Expand All @@ -92,7 +92,7 @@ describe("client", () => {

// expect error on missing 'params'
// @ts-expect-error
await client.GET("/blogposts/{post_id}", {});
await client.GET("/blogposts/{post_id}");

// expect error on empty params
// @ts-expect-error
Expand Down Expand Up @@ -120,7 +120,7 @@ describe("client", () => {

// expet error on missing header
// @ts-expect-error
await client.GET("/header-params", {});
await client.GET("/header-params");

// expect error on incorrect header
// @ts-expect-error
Expand Down Expand Up @@ -235,7 +235,7 @@ describe("client", () => {

// expect error on missing `body`
// @ts-expect-error
await client.PUT("/blogposts", {});
await client.PUT("/blogposts");

// expect error on missing fields
// @ts-expect-error
Expand Down Expand Up @@ -271,7 +271,7 @@ describe("client", () => {
const client = createClient<paths>();

// assert missing `body` doesn’t raise a TS error
await client.PUT("/blogposts-optional", {});
await client.PUT("/blogposts-optional");

// assert error on type mismatch
// @ts-expect-error
Expand All @@ -294,13 +294,13 @@ describe("client", () => {
it("respects baseUrl", async () => {
let client = createClient<paths>({ baseUrl: "https://myapi.com/v1" });
mockFetch({ status: 200, body: JSON.stringify({ message: "OK" }) });
await client.GET("/self", {});
await client.GET("/self");

// assert baseUrl and path mesh as expected
expect(fetchMocker.mock.calls[0][0]).toBe("https://myapi.com/v1/self");

client = createClient<paths>({ baseUrl: "https://myapi.com/v1/" });
await client.GET("/self", {});
await client.GET("/self");
// assert trailing '/' was removed
expect(fetchMocker.mock.calls[1][0]).toBe("https://myapi.com/v1/self");
});
Expand All @@ -310,7 +310,7 @@ describe("client", () => {

const client = createClient<paths>({ headers });
mockFetchOnce({ status: 200, body: JSON.stringify({ email: "[email protected]" }) });
await client.GET("/self", {});
await client.GET("/self");

// assert default headers were passed
const options = fetchMocker.mock.calls[0][1];
Expand Down Expand Up @@ -359,7 +359,7 @@ describe("client", () => {
const client = createClient<paths>({
fetch: async () => Promise.resolve(customFetch as Response),
});
expect((await client.GET("/self", {})).data).toBe(data);
expect((await client.GET("/self")).data).toBe(data);
});
});

Expand Down Expand Up @@ -426,7 +426,7 @@ describe("client", () => {
it("treats `default` as an error", async () => {
const client = createClient<paths>({ headers: { "Cache-Control": "max-age=10000000" } });
mockFetchOnce({ status: 500, headers: { "Content-Type": "application/json" }, body: JSON.stringify({ code: 500, message: "An unexpected error occurred" }) });
const { error } = await client.GET("/default-as-error", {});
const { error } = await client.GET("/default-as-error");

// discard `data` object
if (!error) throw new Error("treats `default` as an error: error response should be present");
Expand Down Expand Up @@ -471,7 +471,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.GET("/anyMethod", {});
await client.GET("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("GET");
});

Expand Down Expand Up @@ -547,7 +547,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.POST("/anyMethod", {});
await client.POST("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("POST");
});

Expand Down Expand Up @@ -599,7 +599,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.DELETE("/anyMethod", {});
await client.DELETE("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("DELETE");
});

Expand Down Expand Up @@ -640,7 +640,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.OPTIONS("/anyMethod", {});
await client.OPTIONS("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("OPTIONS");
});
});
Expand All @@ -649,7 +649,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.HEAD("/anyMethod", {});
await client.HEAD("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("HEAD");
});
});
Expand All @@ -658,7 +658,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.PATCH("/anyMethod", {});
await client.PATCH("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("PATCH");
});
});
Expand All @@ -667,7 +667,7 @@ describe("client", () => {
it("sends the correct method", async () => {
const client = createClient<paths>();
mockFetchOnce({ status: 200, body: "{}" });
await client.TRACE("/anyMethod", {});
await client.TRACE("/anyMethod");
expect(fetchMocker.mock.calls[0][1]?.method).toBe("TRACE");
});
});
Expand Down
63 changes: 42 additions & 21 deletions packages/openapi-fetch/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import type { ErrorResponse, HttpMethod, SuccessResponse, FilterKeys, MediaType, PathsWithMethod, ResponseObjectMap, OperationRequestBodyContent } from "openapi-typescript-helpers";
import type { ErrorResponse, HttpMethod, SuccessResponse, FilterKeys, MediaType, PathsWithMethod, ResponseObjectMap, OperationRequestBodyContent, HasRequiredKeys } from "openapi-typescript-helpers";

// settings & const
const DEFAULT_HEADERS = {
"Content-Type": "application/json",
};
const TRAILING_SLASH_RE = /\/*$/;

// Note: though "any" is considered bad practice in general, this library relies
// on "any" for type inference only it can give. Same goes for the "{}" type.
Expand Down Expand Up @@ -46,11 +45,16 @@ export type RequestOptions<T> = ParamsOption<T> &
export default function createClient<Paths extends {}>(clientOptions: ClientOptions = {}) {
const { fetch = globalThis.fetch, querySerializer: globalQuerySerializer, bodySerializer: globalBodySerializer, ...options } = clientOptions;

let baseUrl = options.baseUrl ?? "";
if (baseUrl.endsWith("/")) {
baseUrl = baseUrl.slice(0, -1); // remove trailing slash
}

async function coreFetch<P extends keyof Paths, M extends HttpMethod>(url: P, fetchOptions: FetchOptions<M extends keyof Paths[P] ? Paths[P][M] : never>): Promise<FetchResponse<M extends keyof Paths[P] ? Paths[P][M] : unknown>> {
const { headers, body: requestBody, params = {}, parseAs = "json", querySerializer = globalQuerySerializer ?? defaultQuerySerializer, bodySerializer = globalBodySerializer ?? defaultBodySerializer, ...init } = fetchOptions || {};

// URL
const finalURL = createFinalURL(url as string, { baseUrl: options.baseUrl, params, querySerializer });
const finalURL = createFinalURL(url as string, { baseUrl, params, querySerializer });
const finalHeaders = mergeHeaders(DEFAULT_HEADERS, clientOptions?.headers, headers, (params as any).header);

// fetch!
Expand Down Expand Up @@ -89,38 +93,55 @@ export default function createClient<Paths extends {}>(clientOptions: ClientOpti
return { error, response: response as any };
}

type GetPaths = PathsWithMethod<Paths, "get">;
type PutPaths = PathsWithMethod<Paths, "put">;
type PostPaths = PathsWithMethod<Paths, "post">;
type DeletePaths = PathsWithMethod<Paths, "delete">;
type OptionsPaths = PathsWithMethod<Paths, "options">;
type HeadPaths = PathsWithMethod<Paths, "head">;
type PatchPaths = PathsWithMethod<Paths, "patch">;
type TracePaths = PathsWithMethod<Paths, "trace">;
type GetFetchOptions<P extends GetPaths> = FetchOptions<FilterKeys<Paths[P], "get">>;
type PutFetchOptions<P extends PutPaths> = FetchOptions<FilterKeys<Paths[P], "put">>;
type PostFetchOptions<P extends PostPaths> = FetchOptions<FilterKeys<Paths[P], "post">>;
type DeleteFetchOptions<P extends DeletePaths> = FetchOptions<FilterKeys<Paths[P], "delete">>;
type OptionsFetchOptions<P extends OptionsPaths> = FetchOptions<FilterKeys<Paths[P], "options">>;
type HeadFetchOptions<P extends HeadPaths> = FetchOptions<FilterKeys<Paths[P], "head">>;
type PatchFetchOptions<P extends PatchPaths> = FetchOptions<FilterKeys<Paths[P], "patch">>;
type TraceFetchOptions<P extends TracePaths> = FetchOptions<FilterKeys<Paths[P], "trace">>;

return {
/** Call a GET endpoint */
async GET<P extends PathsWithMethod<Paths, "get">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "get">>) {
return coreFetch<P, "get">(url, { ...init, method: "GET" } as any);
async GET<P extends GetPaths>(url: P, ...init: HasRequiredKeys<GetFetchOptions<P>> extends never ? [GetFetchOptions<P>?] : [GetFetchOptions<P>]) {
return coreFetch<P, "get">(url, { ...init[0], method: "GET" } as any);
Copy link
Contributor Author

@drwpow drwpow Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The fix was weird, and not straightforward. Essentially, it involved multiple complicated steps:

  1. First, convert the function parameters into an array (...init) (though for simplicity, we can do this starting at the 2nd param, rather than having to do it for the full spread)
  2. Next, we’ll need to take each method’s individual FetchOptions and introspect whether it has a single required key. (we needed to create a helper for this)
  3. Lastly, expand the parameter array into a tuple type, marking it either optional ([T?]) or not ([T]) based on the result of the previous step

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, all of this is very annoying, very strange TypeScript introspection. But that’s the name of the game here, so it’s all worth it… I guess.

},
/** Call a PUT endpoint */
async PUT<P extends PathsWithMethod<Paths, "put">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "put">>) {
return coreFetch<P, "put">(url, { ...init, method: "PUT" } as any);
async PUT<P extends PutPaths>(url: P, ...init: HasRequiredKeys<PutFetchOptions<P>> extends never ? [PutFetchOptions<P>?] : [PutFetchOptions<P>]) {
return coreFetch<P, "put">(url, { ...init[0], method: "PUT" } as any);
},
/** Call a POST endpoint */
async POST<P extends PathsWithMethod<Paths, "post">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "post">>) {
return coreFetch<P, "post">(url, { ...init, method: "POST" } as any);
async POST<P extends PostPaths>(url: P, ...init: HasRequiredKeys<PostFetchOptions<P>> extends never ? [PostFetchOptions<P>?] : [PostFetchOptions<P>]) {
return coreFetch<P, "post">(url, { ...init[0], method: "POST" } as any);
},
/** Call a DELETE endpoint */
async DELETE<P extends PathsWithMethod<Paths, "delete">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "delete">>) {
return coreFetch<P, "delete">(url, { ...init, method: "DELETE" } as any);
async DELETE<P extends DeletePaths>(url: P, ...init: HasRequiredKeys<DeleteFetchOptions<P>> extends never ? [DeleteFetchOptions<P>?] : [DeleteFetchOptions<P>]) {
return coreFetch<P, "delete">(url, { ...init[0], method: "DELETE" } as any);
},
/** Call a OPTIONS endpoint */
async OPTIONS<P extends PathsWithMethod<Paths, "options">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "options">>) {
return coreFetch<P, "options">(url, { ...init, method: "OPTIONS" } as any);
async OPTIONS<P extends OptionsPaths>(url: P, ...init: HasRequiredKeys<OptionsFetchOptions<P>> extends never ? [OptionsFetchOptions<P>?] : [OptionsFetchOptions<P>]) {
return coreFetch<P, "options">(url, { ...init[0], method: "OPTIONS" } as any);
},
/** Call a HEAD endpoint */
async HEAD<P extends PathsWithMethod<Paths, "head">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "head">>) {
return coreFetch<P, "head">(url, { ...init, method: "HEAD" } as any);
async HEAD<P extends HeadPaths>(url: P, ...init: HasRequiredKeys<HeadFetchOptions<P>> extends never ? [HeadFetchOptions<P>?] : [HeadFetchOptions<P>]) {
return coreFetch<P, "head">(url, { ...init[0], method: "HEAD" } as any);
},
/** Call a PATCH endpoint */
async PATCH<P extends PathsWithMethod<Paths, "patch">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "patch">>) {
return coreFetch<P, "patch">(url, { ...init, method: "PATCH" } as any);
async PATCH<P extends PatchPaths>(url: P, ...init: HasRequiredKeys<PatchFetchOptions<P>> extends never ? [PatchFetchOptions<P>?] : [PatchFetchOptions<P>]) {
return coreFetch<P, "patch">(url, { ...init[0], method: "PATCH" } as any);
},
/** Call a TRACE endpoint */
async TRACE<P extends PathsWithMethod<Paths, "trace">>(url: P, init: FetchOptions<FilterKeys<Paths[P], "trace">>) {
return coreFetch<P, "trace">(url, { ...init, method: "TRACE" } as any);
async TRACE<P extends TracePaths>(url: P, ...init: HasRequiredKeys<TraceFetchOptions<P>> extends never ? [TraceFetchOptions<P>?] : [TraceFetchOptions<P>]) {
return coreFetch<P, "trace">(url, { ...init[0], method: "TRACE" } as any);
},
};
}
Expand All @@ -145,8 +166,8 @@ export function defaultBodySerializer<T>(body: T): string {
}

/** Construct URL string from baseUrl and handle path and query params */
export function createFinalURL<O>(url: string, options: { baseUrl?: string; params: { query?: Record<string, unknown>; path?: Record<string, unknown> }; querySerializer: QuerySerializer<O> }): string {
let finalURL = `${options.baseUrl ? options.baseUrl.replace(TRAILING_SLASH_RE, "") : ""}${url as string}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small micro-optimization: we don’t need to be running a dynamic RegEx to remove a trailing slash. It’s much more performant to just remove a single character if needed (done above on createClient(), also to save cycles on running a RegEx every request

export function createFinalURL<O>(pathname: string, options: { baseUrl: string; params: { query?: Record<string, unknown>; path?: Record<string, unknown> }; querySerializer: QuerySerializer<O> }): string {
let finalURL = `${options.baseUrl}${pathname}`;
if (options.params.path) {
for (const [k, v] of Object.entries(options.params.path)) finalURL = finalURL.replace(`{${k}}`, encodeURIComponent(String(v)));
}
Expand Down
3 changes: 3 additions & 0 deletions packages/openapi-typescript-helpers/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,6 @@ export type ErrorResponse<T> = FilterKeys<FilterKeys<T, ErrorStatus>, "content">
export type FilterKeys<Obj, Matchers> = { [K in keyof Obj]: K extends Matchers ? Obj[K] : never }[keyof Obj];
/** Return any `[string]/[string]` media type (important because openapi-fetch allows any content response, not just JSON-like) */
export type MediaType = `${string}/${string}`;
/** Filter objects that have required keys */
export type FindRequiredKeys<T, K extends keyof T> = K extends unknown ? (undefined extends T[K] ? never : K) : K;
export type HasRequiredKeys<T> = FindRequiredKeys<T, keyof T>;