kwhitley / itty-fetcher

An even simpler wrapper around native Fetch to strip boilerplate from your fetching code!
MIT License
99 stars 4 forks source link

Pass FormData in as-is #11

Closed danawoodman closed 2 years ago

danawoodman commented 2 years ago

Closes #10

weepy commented 2 years ago

i did this!

Crisfole commented 2 years ago

@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 commented 2 years ago

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

danawoodman commented 2 years ago

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

Crisfole commented 2 years ago

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

image

danawoodman commented 2 years ago

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 commented 2 years ago

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

kwhitley commented 2 years ago

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

Sources confirming general browser/worker availability

Questions

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 commented 2 years ago

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

image

That screenshot is from just after nvm install v18

danawoodman commented 2 years ago

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

danawoodman commented 2 years ago

@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 commented 2 years ago

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

danawoodman commented 2 years ago

@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