Skip to content
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: support FormData object on fetch body #3417

Merged
merged 5 commits into from
May 20, 2022

Conversation

bholmesdev
Copy link
Contributor

Changes

  • Resolves 🐛 BUG: FormData object causes fetch error #2741
  • Move node-fetch to external dependency for @astrojs/webapi bundle. This was causing strange issues with body parsing that I still haven't figured out. Still, since @astrojs/telemetry already pulls in node-fetch as a standard dependency, this shouldn't be a problem!
  • moving away from using node-fetch/src/index.js exposed type issues. A few pieces to resolve:
    • fetch no longer accepts URL as a resource type. Funny enough, you can still pass a URL for node-fetch to run toString() implicitly. Still, you get red squigglies in VS Code using URL today, so removing this type option is for consistency if anything.
    • FetchInit now uses node-fetch's body type in its type signature

Why not switch to undici like all the cool kids?

@FredKSchott (a cool kid) suggested using undici since it mirrors Node's incoming fetch standard. This seemed possible at first... until I discovered their node versioning requirements. In short: they don't expose fetch or Request for node versions below 16.5. Also poked the source code to confirm. This would mean dropping Node 14.X support, which feels a bit too far as we approach 1.0. Keep it in mind for the future though!

Testing

N/A

Docs

N/A

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm

@changeset-bot
Copy link

changeset-bot bot commented May 20, 2022

🦋 Changeset detected

Latest commit: 3174bde

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

This PR includes changesets to release 6 packages
Name Type
@astrojs/webapi Minor
astro Patch
@astrojs/netlify Patch
@astrojs/node Patch
@astrojs/vercel Patch
astro-scripts 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

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

LGTM! I would change the changeset to minor for webapi though.

.changeset/serious-vans-smell.md Outdated Show resolved Hide resolved
@bholmesdev bholmesdev merged commit 4de53ec into main May 20, 2022
@bholmesdev bholmesdev deleted the fix/support-formdata-object branch May 20, 2022 21:26
@github-actions github-actions bot mentioned this pull request May 20, 2022
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.

🐛 BUG: FormData object causes fetch error
3 participants