Fix body handling on the Request constructor#458
Conversation
|
@domenic, can you take a look? I want to use a transform stream and pipeTo to avoid manual stream creation. Does it make sense to you? I filed whatwg/streams#658 for this change. Thanks! |
annevk
added
the
needs tests
|
This seems a little confusing to me because it mixes the conceptual level (linking to the model section for transform stream) with the concrete level (linking to the specific algorithm for pipeTo). Maybe it is OK because we are just saying "everyone knows what an identity transform stream is". But it's not 100% clear to me, e.g. what the error propagation behavior is. So in other words, the intent seems good, but it might not be precise enough. I'm curious what @annevk thinks. Maybe one direction would be to make things even more vague, e.g. not mention the pipeTo algorithm at all, but just kind of vaguely describe how to get |
|
So my main problem is, what happens if the developer overrides |
|
The idea would be to create an abstract op for pipeTo (whatwg/streams#658) so that calling it always calls the spec algorithm. Not sure where that falls on your safe vs. abstracted spectrum :) |
|
Sorry I missed that. I think that's okay. Could Streams not also define an identity transform stream that can be passed to that algorithm? Perhaps with a note that it's only for use by specifications until someone figures out a use case to expose it to JavaScript? |
|
Yeah, maybe that is the way to go. Then we can define error handling behavior more concretely. We definitely have a use case for it, we just haven't yet specified all the underlying infrastructure (e.g. there are no algorithms for transform streams yet). |
fetch.bs
Outdated
| <a href=https://streams.spec.whatwg.org/#ts-model>transform stream</a> and | ||
| <a href=https://streams.spec.whatwg.org/#pipe-chains>piping</a> are precisely defined. | ||
| This makes <var>inputBody</var>'s <a for=body>stream</a> <a for=ReadableStream>locked</a> and | ||
| <a for=ReadableStream>disturbed</a> immediately. |
There was a problem hiding this comment.
I suggest we file an issue against Fetch to track this. And make this a class=XXX rather than a note.
There was a problem hiding this comment.
Thanks, this PR looks good to me now, but we will need tests per the new process.
|
So I am now a bit confused. Do we want streams to define these things in vague terms? Or is the vague phrasing here, linking to the vague "model" section of streams, good enough for now (until transform streams are defined)? It sounds like we don't need a pipeTo abstract operation anymore? |
|
I think it's okay to be vague for now, but I don't want us to be vague forever, so I want some kind of tracking URL for making it less vague. We also need tests for the overall change. |
yutakahirano
force-pushed
the
new-stream-from-stream
branch
from
9aa9623 to
a45131c
Compare
3 participants
As Request.body is exposed, we need to specify how to handle the body property of the input Request more correctly. After
successfully returns,
request.bodyshould remain unchanged.request.bodyUsedshould be true.Fixing yutakahirano/fetch-with-streams#60.