-
-
Notifications
You must be signed in to change notification settings - Fork 11.5k
fix(fetch-adapter): set correct Content-Type for Node FormData #6998
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
fix(fetch-adapter): set correct Content-Type for Node FormData #6998
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jasonsaayman
left a comment
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.
looks good i just question if need to include chai? i think its unnecessary to include it as we use the built in assertions
|
Alright, thanks for the response, will definitely make the update |
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.
Pull Request Overview
Fixes Content-Type header handling for FormData in Node.js fetch adapter to properly set multipart/form-data with boundary instead of the incorrect application/x-www-form-urlencoded.
- Replaces complex header parsing logic with direct usage of FormData's
getHeaders()method - Adds Node.js-specific test to verify correct
multipart/form-dataContent-Type with boundary - Simplifies browser vs Node.js environment handling
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/helpers/resolveConfig.js | Updates FormData Content-Type handling to use getHeaders() for Node.js FormData |
| test/unit/adapters/fetch.js | Adds test case to verify correct Content-Type header for Node.js FormData |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
jasonsaayman
left a comment
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.
LGTM, thanks for the contribution 🔥
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Looking forward to making more contributions. Any tips on how I can easily understand the folder structure? |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
honestly i only got used to this after a real long time, this is something i hope to vastly improve in the next version as the on-ramp is quite steep and it should not be. but i would say taking up more issue and trying to solve them can help with this |
|
Thank you very much. |
|
Hi, @emiedonmokumo! This PR has been published in v1.12.0 release. Thank you for your contribution ❤️! |
Summary
This pull request fixes an issue in the fetch adapter for Node.js where FormData requests were being sent with
application/x-www-form-urlencodedinstead ofmultipart/form-data; boundary=....Problem
In Node.js, the fetch adapter did not correctly set the
Content-Typeheader when sending FormData.This caused requests using FormData to fail or be misinterpreted by servers expecting
multipart/form-datawith the proper boundary.The issue was confirmed in axios#6993 and affects only the Node.js environment; browser behavior remains correct.
Solution
getHeaders()method (from theform-datapackage).data.getHeaders()['content-type']to get the correctContent-Typeheader including the boundary.form-datapackage to verify theContent-Typeheader is correctly set with a boundary: