-
-
Notifications
You must be signed in to change notification settings - Fork 529
Allow falsy values (except undefined) as a valid body #1825
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"openapi-typescript": patch | ||
--- | ||
|
||
Allow falsy values (except undefined) as a valid body |
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, | ||||
|
@@ -1335,6 +1337,186 @@ describe("client", () => { | |||
}); | ||||
}); | ||||
|
||||
describe("body serialization", () => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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({ | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should definitely use more often What about a single test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
I usually prefer the assertions to be as explicit as possible, hence the explicit |
||||
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 }); | ||||
|
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 😢
There was a problem hiding this comment.
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