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.

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

Conversation

thatsprettyfaroutman
Copy link
Contributor

I was trying to use openapi-fetch for file uploading, but by default that wasn't possible (using multipart/form-data). Looking at the source code I noticed that part of the solution for fixing it seemed to be missing as defaultBodySerializer never allows FormData to be passed through it:

if (requestInit.body) {
  // Here, bodySerializer always returns a JSON string by default
  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) {
    requestInit.headers.delete("Content-Type");
  }
}

I'm trying to fix that in this PR by allowing FormData to be passed through the defaultBodySerializer enabling file uploads with multipart/form-data by default.

Changes

Allow FormData to be passed through defaultBodySerializer as body to enable file uploading by default.

How to Review

Upload a file as part of multipart/form-data

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

@thatsprettyfaroutman thatsprettyfaroutman requested a review from a team as a code owner July 15, 2024 11:02
Copy link

changeset-bot bot commented Jul 15, 2024

🦋 Changeset detected

Latest commit: 0abd93c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@drwpow
Copy link
Contributor

drwpow commented Jul 15, 2024

Thank you for this fix! I’d accept this change if you also added a test that was failing before your fix, and passes with.

Also be sure to add a Changeset for this that adds a patch to openapi-fetch so it can be released on merge!

@thatsprettyfaroutman thatsprettyfaroutman force-pushed the feat-body-serializer-allow-formdata branch from f780936 to 2a96476 Compare July 18, 2024 18:48
});
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.

@thatsprettyfaroutman
Copy link
Contributor Author

thatsprettyfaroutman commented Jul 18, 2024

Thanks @drwpow! I added the test and a changeset entry

@drwpow
Copy link
Contributor

drwpow commented Jul 19, 2024

Looks good! But I see we have a type error in the tests we should probably fix in openapi-fetch/index.d.ts.

@drwpow drwpow merged commit 7698546 into openapi-ts:main Aug 1, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Aug 1, 2024
@thatsprettyfaroutman thatsprettyfaroutman deleted the feat-body-serializer-allow-formdata branch October 21, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants