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

Allow falsy values (except undefined) as a valid body #1825

Merged
merged 2 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/rich-crews-repeat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"openapi-typescript": patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops! Didn’t catch this was for the wrong package. Should be openapi-fetch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh... crap. Sorry. I'll issue a fix PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no, no worries; that’s easy to fix. It’s actually more important that you created the file in the correct PR, because then the changelog credits you correctly and links to the PR. But just pointing out for next time

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 see it has been already released. 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Yours was released correctly! 🙂 #1889

The openapi-typescript release was for an actual patch from another PR: #1890

---

Allow falsy values (except undefined) as a valid body
2 changes: 1 addition & 1 deletion packages/openapi-fetch/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export default function createClient(clientOptions) {
...init,
headers: mergeHeaders(baseHeaders, headers, params.header),
};
if (requestInit.body) {
if (requestInit.body !== undefined) {
requestInit.body = bodySerializer(requestInit.body);
// remove `Content-Type` if serialized body is FormData; browser will correctly set Content-Type & boundary expression
if (requestInit.body instanceof FormData) {
Expand Down
182 changes: 182 additions & 0 deletions packages/openapi-fetch/test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { HttpResponse, type StrictResponse } from "msw";
import { afterAll, beforeAll, describe, expect, expectTypeOf, it } from "vitest";
import createClient, {
type BodySerializer,
type FetchOptions,
type MethodResponse,
type Middleware,
type MiddlewareCallbackParams,
Expand Down Expand Up @@ -1335,6 +1337,186 @@ describe("client", () => {
});
});

describe("body serialization", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these tests could be better placed next to other body tests

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 originally had them there, but then saw that the whole test group is within "TypeScript checks" and these have nothing to do with TypeScript.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right and it seems that most of the tests in this group are not even about types 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s a good callout. We probably need to clean up our tests by moving TypeScript tests to actual Vitest type tests. This test file has also grown in size (which is a great problem to have) and can probably be broken up. All that to say, not too prescriptive about where this test goes; if it exists somewhere I’m happy 🙂

const BODY_ACCEPTING_METHODS = [["PUT"], ["POST"], ["DELETE"], ["OPTIONS"], ["PATCH"]] as const;
const ALL_METHODS = [...BODY_ACCEPTING_METHODS, ["GET"], ["HEAD"]] as const;

const fireRequestAndGetBodyInformation = async (options: {
bodySerializer?: BodySerializer<unknown>;
method: (typeof ALL_METHODS)[number][number];
fetchOptions: FetchOptions<any>;
}) => {
const client = createClient<any>({ baseUrl, bodySerializer: options.bodySerializer });
const { getRequest } = useMockRequestHandler({
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 assume this is piling one mock over the other. I didn't find any "cleanup" procedure in the existing tests, so piling several more mocks on top 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, missed that. Thanks 👍

baseUrl,
method: "all",
path: "/blogposts-optional",
status: 200,
});
await client[options.method]("/blogposts-optional", options.fetchOptions as any);

const request = getRequest();
const bodyText = await request.text();

return { bodyUsed: request.bodyUsed, bodyText };
};

it.each(ALL_METHODS)("missing body (with body serializer) - %s", async (method) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should definitely use more often it.each to have cleaner tests while testing more cases. But in this case isn't it overkill to test every method each time as the logic tested is only about falsy values?

What about a single test it.each([{ body: undefined, except: false }, { body: 1, except: true }, ...])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in this case isn't it overkill to test every method each time as the logic tested is only about falsy values?

There are differences in how GET and HEAD are used in some of the underlying implementation (not in this lib, hopefully) - some clients and servers disallow bodies for them. This includes the tools used in these tests. Having to think about these differences inspired me to rather have a comprehensive test of all methods. If you guys don't see it valuable, I'm OK with dropping it.

What about a single test it.each([{ body: undefined, except: false }, { body: 1, except: true }, ...])

I usually prefer the assertions to be as explicit as possible, hence the explicit expect().toBe(), but I don't mind pulling the expectations into the test arguments, if that's what you guys are used to. Matter of taste really.

const bodySerializer = vi.fn((body) => `Serialized: ${JSON.stringify(body)}`);
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
bodySerializer,
method,
fetchOptions: {},
});

expect(bodyUsed).toBe(false);
expect(bodyText).toBe("");
expect(bodySerializer).not.toBeCalled();
});

it.each(ALL_METHODS)("missing body (without body serializer) - %s", async (method) => {
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({ method, fetchOptions: {} });

expect(bodyUsed).toBe(false);
expect(bodyText).toBe("");
});

it.each(ALL_METHODS)("`undefined` body (with body serializer) - %s", async (method) => {
const bodySerializer = vi.fn((body) => `Serialized: ${JSON.stringify(body)}`);
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
bodySerializer,
method,
fetchOptions: {
body: undefined,
},
});

expect(bodyUsed).toBe(false);
expect(bodyText).toBe("");
expect(bodySerializer).not.toBeCalled();
});

it.each(ALL_METHODS)("`undefined` body (without body serializer) - %s", async (method) => {
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
method,
fetchOptions: {
body: undefined,
},
});

expect(bodyUsed).toBe(false);
expect(bodyText).toBe("");
});

it.each(BODY_ACCEPTING_METHODS)("`null` body (with body serializer) - %s", async (method) => {
const bodySerializer = vi.fn((body) => `Serialized: ${JSON.stringify(body)}`);
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
bodySerializer,
method,
fetchOptions: {
body: null,
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe("Serialized: null");
expect(bodySerializer).toBeCalled();
});

it.each(BODY_ACCEPTING_METHODS)("`null` body (without body serializer) - %s", async (method) => {
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
method,
fetchOptions: {
body: null,
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe("null");
});

it.each(BODY_ACCEPTING_METHODS)("`false` body (with body serializer) - %s", async (method) => {
const bodySerializer = vi.fn((body) => `Serialized: ${JSON.stringify(body)}`);
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
bodySerializer,
method,
fetchOptions: {
body: false,
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe("Serialized: false");
expect(bodySerializer).toBeCalled();
});

it.each(BODY_ACCEPTING_METHODS)("`false` body (without body serializer) - %s", async (method) => {
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
method,
fetchOptions: {
body: false,
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe("false");
});

it.each(BODY_ACCEPTING_METHODS)("`''` body (with body serializer) - %s", async (method) => {
const bodySerializer = vi.fn((body) => `Serialized: ${JSON.stringify(body)}`);
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
bodySerializer,
method,
fetchOptions: {
body: "",
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe('Serialized: ""');
expect(bodySerializer).toBeCalled();
});

it.each(BODY_ACCEPTING_METHODS)("`''` body (without body serializer) - %s", async (method) => {
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
method,
fetchOptions: {
body: "",
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe('""');
});

it.each(BODY_ACCEPTING_METHODS)("`0` body (with body serializer) - %s", async (method) => {
const bodySerializer = vi.fn((body) => `Serialized: ${JSON.stringify(body)}`);
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
bodySerializer,
method,
fetchOptions: {
body: 0,
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe("Serialized: 0");
expect(bodySerializer).toBeCalled();
});

it.each(BODY_ACCEPTING_METHODS)("`0` body (without body serializer) - %s", async (method) => {
const { bodyUsed, bodyText } = await fireRequestAndGetBodyInformation({
method,
fetchOptions: {
body: 0,
},
});

expect(bodyUsed).toBe(true);
expect(bodyText).toBe("0");
});
});

describe("requests", () => {
it("multipart/form-data", async () => {
const client = createClient<paths>({ baseUrl });
Expand Down