Skip to content

feat: allow form data through default body serializer #1762

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 6 commits into from
Aug 1, 2024
Merged

feat: allow form data through default body serializer #1762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59d3240
feat: allow form data through default body serializer
thatsprettyfaroutman Jul 15, 2024
2d3418b
chore: lint
thatsprettyfaroutman Jul 18, 2024
2a96476
test: send multipart/form-data with file
thatsprettyfaroutman Jul 18, 2024
f9b5657
docs: openapi-fetch changeset patch
thatsprettyfaroutman Jul 18, 2024
724cd67
test: cast FormData to string instead of ts-ignore
thatsprettyfaroutman Jul 18, 2024
0abd93c
chore: lint
thatsprettyfaroutman Jul 18, 2024
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/warm-plums-wait.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"openapi-fetch": patch
---

Allow FormData through defaultBodySerializer
3 changes: 3 additions & 0 deletions packages/openapi-fetch/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,9 @@ export function defaultPathSerializer(pathname, pathParams) {
* @type {import("./index.js").defaultBodySerializer}
*/
export function defaultBodySerializer(body) {
if (body instanceof FormData) {
return body;
}
return JSON.stringify(body);
}

Expand Down
40 changes: 40 additions & 0 deletions packages/openapi-fetch/test/fixtures/api.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,46 @@ export interface paths {
patch?: never;
trace?: never;
};
"/multipart-form-data-file-upload": {
parameters: {
query?: never;
header?: never;
path?: never;
cookie?: never;
};
get?: never;
put?: never;
post: {
parameters: {
query?: never;
header?: never;
path?: never;
cookie?: never;
};
requestBody: {
content: {
"multipart/form-data": string;
};
};
responses: {
200: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": {
text: string;
};
};
};
};
};
delete?: never;
options?: never;
head?: never;
patch?: never;
trace?: never;
};
}
export type webhooks = Record<string, never>;
export interface components {
Expand Down
20 changes: 20 additions & 0 deletions packages/openapi-fetch/test/fixtures/api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,26 @@ paths:
responses:
200:
$ref: "#/components/responses/MultipleResponse"
/multipart-form-data-file-upload:
post:
requestBody:
content:
multipart/form-data:
schema:
type: string
format: binary
required: true
responses:
"200":
content:
application/json:
schema:
type: object
properties:
text:
type: string
required:
- text
components:
schemas:
Post:
Expand Down
27 changes: 27 additions & 0 deletions packages/openapi-fetch/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,6 +1352,33 @@ describe("client", () => {
customProperty: "value",
});
});

it("multipart/form-data with a file", async () => {
const TEST_STRING = "Hello this is text file string";

const file = new Blob([TEST_STRING], { type: "text/plain" });
const formData = new FormData();
formData.append("file", file);

const client = createClient<paths>({ baseUrl });
useMockRequestHandler({
baseUrl,
method: "post",
path: "/multipart-form-data-file-upload",
handler: async (data) => {
// Get text from file and send it back
const formData = await data.request.formData();
const text = await (formData.get("file") as File).text();
return new HttpResponse(JSON.stringify({ text }));
},
});
const { data } = await client.POST("/multipart-form-data-file-upload", {
// TODO: how to get this to accept FormData?
body: formData as unknown 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.

How to get this to accept FormData?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh hm do we need to adjust the types here? Is there an improvement we could make in packages/openapi-fetch/index.d.ts for body?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this will be tricky. This library’s whole purpose is to assert types for body based on your OpenAPI schema. https://github.com/openapi-ts/openapi-typescript/blob/main/packages/openapi-fetch/src/index.d.ts#L90

We don’t want to always allow FormData as that would mess up some people’s code. So we want to conditionally allow FormData in some instances where it’s possible (e.g. not for GET). So we’ll need to surgically add FormData as a union into request bodies, but only when it may be possible/desired.

Given the complexity, I’m OK with merging this PR without that just to unblock the runtime, and the types can be tackled separately (sidenote: that’s why the runtime and types are separated—the type complexity isn’t possible to express with normal runtime code)

Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts? Do you want to try and solve the type errors? Or merge by requiring manual overrides?

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'm ok with merging now. I think letting FormData through the body serializer is a nice little addition in itself. Oh, I was wondering why the types are separated, that's very cool to know 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Will merge.

});

expect(data?.text).toBe(TEST_STRING);
});
});

describe("responses", () => {
Expand Down