Closed grantila closed 6 years ago
Only trailers left. The rest are failure now, they shouldn't happen. Will keep open until trailers are completely implemented.
Whoops, sorry for not responding to this earlier.
I agree with the optional argument in the fetch call for trailers
. something like handleTrailers
might be intuitive? Definitely seems intuitive to me.
I also don't think I completely understand how continue
works (will need to read up about this).
With headers
is it possible to receive a valid response after receiving a 1xx
response? If not, then yeah, rejecting might make sense.
In general, It's likely fine to reject a promise as long as we're providing a user some way to identify which one of these things happened from the error object they receive in .catch
Or onTrailers
? Semantics, but still. Perhaps just trailers
. We already have onPush
.
You're right, it would be very useful to be able to specifically catch these errors, so their constructors must be public. I'll fix that.
Won't close this until all these issues are well defined and documented.
timeout
and aborted
errors will be handled in #6. Other errors are rare and are pure Error
's. Should likely never happen anyway.
That makes sense.
I wonder if frameError
is a valid error that could be the client's fault, and not the backend?,
In any case, yeah, frameError
should rejected the promise with an Error
. I think the implementation is correct
By the way, @pranaygp, you mentioned the body
handling previously, this is changed in #5, so now you can send any string, buffer and readable stream as {body}
, and a helper {json}
for objects that should be sent as JSON (with application/json
automatically set).
I noticed that, and was actually just reading the changes
I don't think getStringOrBufferOrReadableStream
is a good name though
(from the README)
How does fetch handle this? Can we just have a single body
property in
the object that takes a string, or array buffer or stream. And then, the
user can just add content-type: application/json
to the headers if they
want to.
That way we can completely get rid of constructors the user needs to
invoke, and stay closer to fetch
On Fri, 3 Nov 2017, 14:46 Gustaf Räntilä, notifications@github.com wrote:
By the way, @pranaygp https://github.com/pranaygp, you mentioned the body handling previously, this is changed in #5 https://github.com/grantila/fetch-h2/issues/5, so now you can send any string, buffer and readable stream as {body}, and a helper {json} for objects that should be sent as JSON (with application/json automatically set).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/grantila/fetch-h2/issues/4#issuecomment-341808049, or mute the thread https://github.com/notifications/unsubscribe-auth/ABtutDRGYyVJ6VtHBLAACd4Ijyn5-tcgks5sy22vgaJpZM4QMeN0 .
That's exactly how it's done now, maybe I was unclear in the docs, I'll update it. I meant that you can assign these types to body
directly.
As said in #1, there are Http2Stream events currently not handled. There are different reasons for all of them to not yet having been handled, this is my current thoughts:
aborted
error
, i.e. could we ignore this?frameError
timeout
trailers
response
, which is when the promise is resolved andfetch()
is complete (not the body data necessarily). If this should be handled at all, we'd need an optional callback in the options argument tofetch()
so that the user can get these trailing headers. Implementation logic (such as cookies) are not allowed to be sent, sofetch-h2
won't internally need to care about these headers.continue
Expect
header to be sent in a request, so 100 shouldn't be a valid response. This event should probably reject the flow as it can't happen.headers
fetch()
would ever expect a 1xx response, this is usually a set of low-level handshakes. I don't think this should be handled other than perhaps a rejected promise (at least not for now).Thoughts on this @pranaygp?