Handle all (relevant) Http2Stream events

[grantila]

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
  • Will this also invoke error, i.e. could we ignore this?
• frameError
  • Same
• timeout
  • Same
• trailers
  • AFAIK, these can occur after response, which is when the promise is resolved and fetch() is complete (not the body data necessarily). If this should be handled at all, we'd need an optional callback in the options argument to fetch() so that the user can get these trailing headers. Implementation logic (such as cookies) are not allowed to be sent, so fetch-h2 won't internally need to care about these headers.
• continue
  • The Fetch API clearly forbids the 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
  • I'm not sure why a user of 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?

[grantila]

Only trailers left. The rest are failure now, they shouldn't happen. Will keep open until trailers are completely implemented.

[pranaygp]

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

[grantila]

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.

[grantila]

timeout and aborted errors will be handled in #6. Other errors are rare and are pure Error's. Should likely never happen anyway.

[pranaygp]

That makes sense.

I wonder if frameError is a valid error that could be the client's fault, and not the backend?,

[pranaygp]

In any case, yeah, frameError should rejected the promise with an Error. I think the implementation is correct

[grantila]

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).

[pranaygp]

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ä, ***@***.***> wrote: By the way, @pranaygp <https://github.com/pranaygp>, you mentioned the body handling previously, this is changed in #5 <#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 <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABtutDRGYyVJ6VtHBLAACd4Ijyn5-tcgks5sy22vgaJpZM4QMeN0> .

[grantila]

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.

[grantila]

😎 bf5028f