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

Pass FormData in as-is #11

Merged
merged 5 commits into from Oct 5, 2022

Conversation

danawoodman
Copy link
Collaborator

@danawoodman danawoodman commented Oct 4, 2022

  • Add support for passing in FormData as-is instead of JSON.stringifying it as we do now. Should only add a few bytes to the final bundle
  • Add tests for passing in FormData
  • Big refactoring of tests

Closes #10

@weepy
Copy link

weepy commented Oct 5, 2022

i did this!

@Crisfole
Copy link
Contributor

Crisfole commented Oct 5, 2022

@danawoodman you need to stub in FormData or run it in an environment like MiniFlare or CF Workerd where FormData is stubbed in for you.

@danawoodman
Copy link
Collaborator Author

@Crisfole Why would it not need to be stubbed locally tho? I'm running the same build/test commands

@danawoodman
Copy link
Collaborator Author

This also brings up a question: if FormData isn't available, itty-fetcher will fail; what can we do about that?

@Crisfole
Copy link
Contributor

Crisfole commented Oct 5, 2022

Request is also not globally available. Nor is Response. This is Not Your Problem:tm:.

image

@danawoodman
Copy link
Collaborator Author

I assume fetch would fail if those aren't available anyways, without some kind of poly fill/override.

Now how do we fix these tests since they work fine locally?

@Crisfole
Copy link
Contributor

Crisfole commented Oct 5, 2022

@danawoodman what's your local env like? (Node version?)

@kwhitley
Copy link
Owner

kwhitley commented Oct 5, 2022

Yeah, same issue... pulled this in an hit with FormData immediately.
image

Sources confirming general browser/worker availability

Questions

  • Node (and other runtime) compatibility?

Suggested Path

If this works fine in most of those runtimes without polyfills, I'd say we add the supporting types to the tsconfig.json and bypass this issue ourselves. Leave a requirement to polyfill if the user's runtime doesn't support it. Conversely we try/catch, but...

@Crisfole
Copy link
Contributor

Crisfole commented Oct 5, 2022

This will work in SK, Workers, Pages, it will work in > Node 18:

image

That screenshot is from just after nvm install v18

@danawoodman
Copy link
Collaborator Author

I must be using 18. Will confirm when I'm home

@danawoodman
Copy link
Collaborator Author

@kwhitley I've updated the actions to use Node 18 (fixes actions), added a .node-version file to set that locally and added some notes in the readme. I think this is ready for your review!

Thanks @Crisfole for pointing me to the solution!

@kwhitley
Copy link
Owner

kwhitley commented Oct 5, 2022

This is awesome, only comment is that thing about 18.0.0 vs 18.10.0, but i'll merge and tweak that :)

@kwhitley kwhitley merged commit 5ad92fb into kwhitley:v0.x Oct 5, 2022
@danawoodman danawoodman deleted the 10-add-formdata-handling branch October 5, 2022 18:04
@danawoodman
Copy link
Collaborator Author

@kwhitley thanks! I didn't see a note on 18, what was your question? 18.10.0 is the latest version so I opted for that for local dev. The actual action runs on anything 18+ which I think is ok

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.

FormData handling
4 participants