Add headers constructor option and readonly response wt.headers attribute #713
Conversation
jan-ivar
requested review from
ekinnear,
jesup,
martinthomson,
nidhijaju and
vasilvv
|
Meeting:
|
|
Also cc @annevk to make sure we've not missed anything. |
| :: Once a [=WebTransport session=] has been established, holds the application-level | ||
| headers set by the server, if any. Initially null. | ||
| The getter steps are to return [=this=]'s {{[[Headers]]}}. |
There was a problem hiding this comment.
Would it be clearer if we added some text indicating that these headers are subject to the forbidden request-header list as defined by https://fetch.spec.whatwg.org/#methods section 2.2.2?
|
|
||
| <div algorithm="process a WebTransport fetch response"> | ||
| To <dfn>process a WebTransport fetch response</dfn>, given a |response|, and |congestionControl|, run these steps: | ||
| To <dfn>process a WebTransport fetch response</dfn>, given a |response| and |transport|, run these steps: |
There was a problem hiding this comment.
Nit: consider specifying the types of the arguments here.
| 1. Set |transport|.{{[[State]]}} to `"connected"`. | ||
| 1. Set |transport|.{{[[Session]]}} to |session|. | ||
| 1. Let |headers| be a copy of |response|’s [=response/header list=]. | ||
| 1. Remove from |headers| each header whose name is a [=byte-case-insensitive=] match for `WT-Protocol`. |
There was a problem hiding this comment.
| the application will receive the number of streams it anticipates. | ||
|
|
||
| : <dfn for="WebTransport" attribute>headers</dfn> | ||
| :: Once a [=WebTransport session=] has been established, holds the application-level |
There was a problem hiding this comment.
| :: Once a [=WebTransport session=] has been established, holds the application-level | |
| :: Once a [=WebTransport session=] has been established, it holds the application-level |
?
| 1. Let |headers| be {{WebTransport/constructor(url, options)/options}}["{{WebTransportOptions/headers}}"]. | ||
| 1. If |headers| is a not a {{Headers}} object, then set |headers| to a new {{Headers}} | ||
| object [=Headers/filled=] with | ||
| {{WebTransport/constructor(url, options)/options}}["{{WebTransportOptions/headers}}"]. | ||
| 1. Set |request|'s [=request/header list=] to a copy of |headers|'s [=Headers/header list=]. | ||
| 1. If |request|'s [=request/header list=] [=header list/contains=] `WT-Available-Protocols`, then | ||
| throw a {{TypeError}}. |
There was a problem hiding this comment.
It seems you are not validating the Headers object and just copy all its headers straight onto request. Are you sure that's what you want? @wilaw's comment below suggests you are expecting some kind of validation at least.
This is why the Request constructor for instance creates its own instance always and then attempts to forward the headers (which might end up throwing).
Perhaps we could/should create some kind of abstraction for this which can be shared by Fetch and WebTransport.
There was a problem hiding this comment.
Good catch! We definitely want filtering by the forbidden request header list
| : <dfn for="WebTransport" attribute>headers</dfn> | ||
| :: Once a [=WebTransport session=] has been established, holds the application-level | ||
| headers set by the server, if any. Initially null. | ||
| The getter steps are to return [=this=]'s {{[[Headers]]}}. |
There was a problem hiding this comment.
This is a little hand-wavy. Shouldn't you define somewhere a slot that holds null or a Headers object?
There was a problem hiding this comment.
@annevk isn't that https://pr-preview.s3.amazonaws.com/jan-ivar/webtransport/pull/713.html#dom-webtransport-headers-slot? Or did you mean something else?
There was a problem hiding this comment.
Oh sorry, I missed [[ and ]] and thought this was referring to the object.
|
@vasilvv did you get any feedback from people on this PR? |
3 participants
Fixes #263.
Preview | Diff