-
Notifications
You must be signed in to change notification settings - Fork 337
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
Allow a request body to be a byte sequence/string #1083
Conversation
This simplifies things for several callers. Fixes #1073.
I think we could get rid of https://fetch.spec.whatwg.org/#concept-empty-readablestream too if we wanted to as it only has one caller and that might as well create a promise resolved with an empty byte sequence. I looked elsewhere and I couldn't find specifications using these. E.g., the Encoding Standard depends directly upon Streams. @ricea as maintainer of Streams this might be of slight interest. |
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.
Very nice; nicer than what I was thinking of doing.
Do you think it's worth adding a note for callers that supply strings that they might want to set a Content-Type with a UTF-8 charset, since you're going to UTF-8 encode for them? Or maybe we shouldn't have the string input version, hmmm...
I was thinking that since the note links "safely extract" it's clear it'll be UTF-8 and we don't really do anything else for strings these days (and shouldn't). |
That's reasonable. However, I'd still err on the side of removing the string possibility until we have more than one concrete caller that would use it. It appears the only user would be reporting, which would not get much simpler, because it would just switch from "serialize JSON to bytes" to... I guess manually using %JSON.stringify%? |
Fair, I'll drop that for now. @yutakahirano opinions on removing the empty ReadableStream primitive? |
fetch.bs
Outdated
@@ -2489,8 +2477,16 @@ object <var>stream</var>, run these steps: | |||
</ol> | |||
|
|||
<p>An <dfn export for=ReadableStream id=concept-empty-readablestream>empty</dfn> {{ReadableStream}} | |||
object is the result of <a lt="construct a fixed ReadableStream object">constructing</a> a fixed | |||
{{ReadableStream}} object with an empty list. | |||
object, run these steps: |
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.
This needs rewording if we are to keep the "empty" construct.
I think we need to fix https://fetch.spec.whatwg.org/#dom-body-body too. |
@yutakahirano hey! What fix does that need? Do you think we need to preserve "empty"? Any other thoughts on this PR? |
... or the body definition in https://fetch.spec.whatwg.org/#request-class. https://fetch.spec.whatwg.org/#concept-body-body expects the body to be a body or null, but with this change (concept) request's body is null or a body or a byte sequence. |
Isn't the byte sequence turned into a body well before the |
I'm not sure it is. https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm creates a Request object from a concept request directly at step 21.3.2.
I'm fine with removing https://fetch.spec.whatwg.org/#concept-empty-readablestream if it's unnecessary. |
Ah I was wrong, that is covered by https://whatpr.org/fetch/1083/ad9a84a...ba5c0f3.html#fetching. Sorry for that. |
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.
Still LGTM but @yutakahirano found bugs that I missed so it'd be good to get his final review :)
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
This simplifies things for several callers.
Fixes #1073.
Preview | Diff