-
Notifications
You must be signed in to change notification settings - Fork 47
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
FormData refactor #34
Conversation
Pull Request Test Coverage Report for Build 329
💛 - Coveralls |
cea37ba
to
3b60361
Compare
This is probably a good first step, but I don't think it's following the spec (not that the previous implementation was especially spec-compliant, but we may as well do it right this time around). Check out the FormData and URLSearchParams bits of https://fetch.spec.whatwg.org/#body-mixin Your current implementation looks a lot like what the fetch spec prescribes for URLSearchParams. I'm just learning this now too btw :) Seems like our currently included That's something we have not being doing quite right to be honest. We need to start looking in the fetch spec more to provide a better, more predictable, implementation. If you want to convert your current PR to do the simple, small URLSearchParams fetch body switch case, that's probably more encouraging and shippable in a short amount of time :)
|
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.
Never mind my previous comment, the FormData
is on spec, except for how it's serialized in the body.
I believe the tests should use a Request and match a request precisely with the right serialization.
test/fixtures/apps/form-data.js
Outdated
fly.http.respondWith(function(request) { | ||
const formData = new FormData() | ||
formData.append('foo', 'bar') | ||
return new Response(formData) |
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.
I don't think an example with a Response
is super relevant. I think FormData
will be much more useful in requests.
FormData is now implemented in TypeScript and is API-complete (as seen in https://developer.mozilla.org/en-US/docs/Web/API/FormData), minus the ability to parse DOM tree, as FormData in Fly is more similar to the one found in Service Workers API.
d831758
to
4ea70b6
Compare
This brings
FormData
back.You can just do (it's available globally in the runtime):
or get it straight from the Response:
Implements #16