fetch: Send headers to JS #727
Conversation
js/fetch.ts
Outdated
| break; | ||
| } | ||
| } | ||
| for (; end > 0; --end) { |
There was a problem hiding this comment.
s/end > 0/end > start/
js/fetch.ts
Outdated
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don’t think it’s necessary at this stage to be fully compliant with web fetch - but I appreciate the effort. If we’re to land this code each of these functions should have unit tests.
js/fetch.ts
Outdated
| const header = init[i]; | ||
| if (header.length !== 2) { | ||
| throw new TypeError(); | ||
| } |
There was a problem hiding this comment.
Necessary? Multiple values with the same key are allowed in http headers.
There was a problem hiding this comment.
@ry This implementation already supports multiple values for the same key, header.length !== 2) is based on this https://fetch.spec.whatwg.org/#concept-headers-fill
There was a problem hiding this comment.
Oh ok - then can you add a test where it shows this throwing a TypeError?
js/fetch_test.ts
Outdated
| testPerm({ net: true }, async function fetchHeaders() { | ||
| const response = await fetch("http:https://localhost:4545/package.json"); | ||
| const headers = response.headers; | ||
|
|
js/fetch_test.ts
Outdated
| const headers = response.headers; | ||
|
|
||
| assertEqual(headers.get("Content-Type"), "application/json"); | ||
| }); |
There was a problem hiding this comment.
Add one more for good measure
assert(headers.get("Server").startsWith("SimpleHTTP");
This is a step toward #522,
(Personal note: 7caab7b)