-
-
Notifications
You must be signed in to change notification settings - Fork 529
Do not set content-type on body-less requests #1826
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
80e2b93
880b288
7e78435
15e89bf
fdeecfe
ef8a69e
1812aff
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-fetch": minor | ||
--- | ||
|
||
Do not set content-type on body-less requests |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,15 @@ import createClient, { | |
type BodySerializer, | ||
type FetchOptions, | ||
type MethodResponse, | ||
type HeadersOptions, | ||
type Middleware, | ||
type MiddlewareCallbackParams, | ||
type QuerySerializerOptions, | ||
type Client, | ||
type PathBasedClient, | ||
createPathBasedClient, | ||
} from "../src/index.js"; | ||
import { server, baseUrl, useMockRequestHandler, toAbsoluteURL } from "./fixtures/mock-server.js"; | ||
import { baseUrl, server, toAbsoluteURL, useMockRequestHandler } from "./fixtures/mock-server.js"; | ||
import type { paths } from "./fixtures/api.js"; | ||
|
||
beforeAll(() => { | ||
|
@@ -819,12 +820,7 @@ describe("client", () => { | |
await client.GET("/self"); | ||
|
||
// assert default headers were passed | ||
expect(getRequest().headers).toEqual( | ||
new Headers({ | ||
...headers, // assert new header got passed | ||
"Content-Type": "application/json", // probably doesn’t need to get tested, but this was simpler than writing lots of code to ignore these | ||
}), | ||
); | ||
expect(getRequest().headers).toEqual(new Headers(headers)); | ||
}); | ||
|
||
it("can be overridden", async () => { | ||
|
@@ -850,7 +846,6 @@ describe("client", () => { | |
expect(getRequest().headers).toEqual( | ||
new Headers({ | ||
"Cache-Control": "no-cache", | ||
"Content-Type": "application/json", | ||
}), | ||
); | ||
}); | ||
|
@@ -894,6 +889,139 @@ describe("client", () => { | |
}); | ||
}); | ||
|
||
describe("content-type", () => { | ||
const BODY_ACCEPTING_METHODS = [["PUT"], ["POST"], ["DELETE"], ["OPTIONS"], ["PATCH"]] as const; | ||
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. nit: Is there a reason to have two dims arrays here? 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. This is what the each method officially expects. It's a list of param sets where each set can consist of multiple params. The API supports a flat array, where |
||
const ALL_METHODS = [...BODY_ACCEPTING_METHODS, ["GET"], ["HEAD"]] as const; | ||
|
||
const fireRequestAndGetContentType = async (options: { | ||
defaultHeaders?: HeadersOptions; | ||
method: (typeof ALL_METHODS)[number][number]; | ||
fetchOptions: FetchOptions<any>; | ||
}) => { | ||
const client = createClient<any>({ baseUrl, headers: options.defaultHeaders }); | ||
const { getRequest } = useMockRequestHandler({ | ||
baseUrl, | ||
method: "all", | ||
path: "/blogposts-optional", | ||
status: 200, | ||
}); | ||
await client[options.method]("/blogposts-optional", options.fetchOptions as any); | ||
|
||
const request = getRequest(); | ||
return request.headers.get("content-type"); | ||
}; | ||
|
||
it.each(ALL_METHODS)("no content-type for body-less requests - %s", async (method) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
method, | ||
fetchOptions: {}, | ||
}); | ||
|
||
expect(contentType).toBe(null); | ||
}); | ||
|
||
it.each(ALL_METHODS)("no content-type for `undefined` body requests - %s", async (method) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
method, | ||
fetchOptions: { | ||
body: undefined, | ||
}, | ||
}); | ||
|
||
expect(contentType).toBe(null); | ||
}); | ||
|
||
const BODIES = [{ prop: "a" }, {}, "", "str", null, false, 0, 1, new Date("2024-08-07T09:52:00.836Z")] as const; | ||
const METHOD_BODY_COMBINATIONS = BODY_ACCEPTING_METHODS.flatMap(([method]) => | ||
BODIES.map((body) => [method, body] as const), | ||
); | ||
Comment on lines
+934
to
+937
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. @kerwanp Guessing from #1825 (comment) you won't like this 🤣 Happy to ditch the methods, but I'd rather keep the various bodies as this was broken until #1825. 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. Aha I'm not against this when behavior have to be different between cases, the pattern you use looks good to me! The methods constants could be moved at the top of the file to be reused in other 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. Until they are actually reused in other tests, they IMO make more sense when they are collocated with the test itself. It's just less scrolling to understand how they are used. Anyway this is just my opinion I've grown over the years, nothing I would fight for. I guess we are in agreement that this file already does too much and deserves some larger refactoring and splitting. Let's maybe open a separate issue and discuss a bit what the desired structure could look like? |
||
|
||
it.each(METHOD_BODY_COMBINATIONS)( | ||
"implicit default content-type for body-full requests - %s, %j", | ||
async (method, body) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
method, | ||
fetchOptions: { | ||
body, | ||
}, | ||
}); | ||
|
||
expect(contentType).toBe("application/json"); | ||
}, | ||
); | ||
|
||
it.each(METHOD_BODY_COMBINATIONS)( | ||
"provided default content-type for body-full requests - %s, %j", | ||
async (method, body) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
defaultHeaders: { | ||
"content-type": "application/my-json", | ||
}, | ||
method, | ||
fetchOptions: { | ||
body, | ||
}, | ||
}); | ||
|
||
expect(contentType).toBe("application/my-json"); | ||
}, | ||
); | ||
|
||
it.each(METHOD_BODY_COMBINATIONS)( | ||
"native-fetch default content-type for body-full requests, when default is suppressed - %s, %j", | ||
async (method, body) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
defaultHeaders: { | ||
"content-type": null, | ||
}, | ||
method, | ||
fetchOptions: { | ||
body, | ||
}, | ||
}); | ||
// the fetch implementation won't allow sending a body without content-type, | ||
// and it defaults to `text/plain;charset=UTF-8`, however the actual default value | ||
// is irrelevant and might be flaky across different fetch implementations | ||
// for us, it's important that it's not `application/json` | ||
expect(contentType).not.toBe("application/json"); | ||
}, | ||
); | ||
|
||
it.each(METHOD_BODY_COMBINATIONS)( | ||
"specified content-type for body-full requests - %s, %j", | ||
async (method, body) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
method, | ||
fetchOptions: { | ||
body, | ||
headers: { | ||
"content-type": "application/my-json", | ||
}, | ||
}, | ||
}); | ||
|
||
expect(contentType).toBe("application/my-json"); | ||
}, | ||
); | ||
|
||
it.each(METHOD_BODY_COMBINATIONS)( | ||
"specified content-type for body-full requests, even when default is suppressed - %s, %j", | ||
async (method, body) => { | ||
const contentType = await fireRequestAndGetContentType({ | ||
method, | ||
fetchOptions: { | ||
body, | ||
headers: { | ||
"content-type": "application/my-json", | ||
}, | ||
}, | ||
}); | ||
|
||
expect(contentType).toBe("application/my-json"); | ||
}, | ||
); | ||
}); | ||
|
||
describe("fetch", () => { | ||
it("createClient", async () => { | ||
function createCustomFetch(data: any) { | ||
|
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 forget if I said this already, but thanks for adding this! Little docs improvements go a long way and are always welcome ❤️