Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Make fetch() more standards compliant #3667

Merged
merged 33 commits into from
Feb 3, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d600c2e
Add a non-compliant implementation of fetch's 'redirect'
serverhiccups Jan 13, 2020
cf8ac58
Support creating the different Response types
serverhiccups Jan 13, 2020
7e74ac1
Support the `error` redirect type properly (mostly)
serverhiccups Jan 13, 2020
3e44fa4
Use the correct ResponseType and support nullable response bodies
serverhiccups Jan 13, 2020
2647983
Lint properly
serverhiccups Jan 13, 2020
ccb0b42
Add experimental support for `nofollow`
serverhiccups Jan 13, 2020
32b80fa
Add experimental support for Response.redirect()
serverhiccups Jan 14, 2020
ce013a7
format
serverhiccups Jan 14, 2020
bb12aed
fix .redirect
serverhiccups Jan 14, 2020
a1c2452
test response body filtering
serverhiccups Jan 16, 2020
a352024
Fix an error that came from me misinterpreting the spec
serverhiccups Jan 17, 2020
360e2fb
More tests
serverhiccups Jan 21, 2020
aed52ab
some formatting
serverhiccups Jan 22, 2020
36761cb
brain dump (I know that this doesn't pass tests)
serverhiccups Jan 23, 2020
bc336cb
more changes
serverhiccups Jan 23, 2020
43ff469
Add a non-compliant implementation of fetch's 'redirect'
serverhiccups Jan 13, 2020
6db9c73
Support creating the different Response types
serverhiccups Jan 13, 2020
a0bb468
Support the `error` redirect type properly (mostly)
serverhiccups Jan 13, 2020
d16459d
Use the correct ResponseType and support nullable response bodies
serverhiccups Jan 13, 2020
e683271
Lint properly
serverhiccups Jan 13, 2020
c5e278a
Add experimental support for `nofollow`
serverhiccups Jan 13, 2020
56076c4
Add experimental support for Response.redirect()
serverhiccups Jan 14, 2020
958a332
format
serverhiccups Jan 14, 2020
f5204e7
fix .redirect
serverhiccups Jan 14, 2020
69bc0fd
test response body filtering
serverhiccups Jan 16, 2020
597734e
Fix an error that came from me misinterpreting the spec
serverhiccups Jan 17, 2020
177a63e
More tests
serverhiccups Jan 21, 2020
889bfbd
some formatting
serverhiccups Jan 22, 2020
d5cfd06
brain dump (I know that this doesn't pass tests)
serverhiccups Jan 23, 2020
10539dd
more changes
serverhiccups Jan 23, 2020
02c807d
Merge branch 'make-fetch-more-compliant' of https://github.com/server…
serverhiccups Jan 24, 2020
264209b
Remove 'nofollow' redirect mode
serverhiccups Jan 24, 2020
a62d55d
Merge branch 'master' into make-fetch-more-compliant
bartlomieju Feb 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/js/dom_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,8 +530,8 @@ type RequestDestination =
| "worker"
| "xslt";
type RequestMode = "navigate" | "same-origin" | "no-cors" | "cors";
type RequestRedirect = "follow" | "error" | "manual";
type ResponseType =
type RequestRedirect = "follow" | "nofollow" | "error" | "manual";
export type ResponseType =
| "basic"
| "cors"
| "default"
Expand Down
142 changes: 137 additions & 5 deletions cli/js/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,11 @@ class Body implements domTypes.Body, domTypes.ReadableStream, io.ReadCloser {
}

export class Response implements domTypes.Response {
readonly type = "basic"; // TODO
readonly type: domTypes.ResponseType;
readonly redirected: boolean;
headers: domTypes.Headers;
readonly trailer: Promise<domTypes.Headers>;
readonly body: Body;
readonly body: null | Body;

constructor(
readonly url: string,
Expand All @@ -265,7 +265,8 @@ export class Response implements domTypes.Response {
headersList: Array<[string, string]>,
rid: number,
redirected_: boolean,
body_: null | Body = null
body_: null | Body = null,
readonly type_: null | domTypes.ResponseType = "default"
) {
this.trailer = createResolvable();
this.headers = new Headers(headersList);
Expand All @@ -277,27 +278,129 @@ export class Response implements domTypes.Response {
this.body = body_;
}

if (type_ == null) {
this.type = "default";
} else {
this.type = type_;
if (type_ == "error") {
// spec: https://fetch.spec.whatwg.org/#concept-network-error
this.status = 0;
this.statusText = "";
this.headers = new Headers();
this.body = null;
/* spec for other Response types:
https://fetch.spec.whatwg.org/#concept-filtered-response-basic
Please note that type "basic" is not the same thing as "default".*/
} else if (type_ == "basic") {
for (const h of this.headers) {
/* Forbidden Response-Header Names:
https://fetch.spec.whatwg.org/#forbidden-response-header-name */
if (["set-cookie", "set-cookie2"].includes(h[0].toLowerCase())) {
this.headers.delete(h[0]);
}
}
} else if (type_ == "cors") {
/* CORS-safelisted Response-Header Names:
https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name */
const allowedHeaders = [
"Cache-Control",
"Content-Language",
"Content-Length",
"Content-Type",
"Expires",
"Last-Modified",
"Pragma"
].map((c: string) => c.toLowerCase());
for (const h of this.headers) {
/* Technically this is still not standards compliant because we are
supposed to allow headers allowed in the
'Access-Control-Expose-Headers' header in the 'internal response'
However, this implementation of response doesn't seem to have an
easy way to access the internal response, so we ignore that
header.
TODO(serverhiccups): change how internal responses are handled
so we can do this properly. */
if (!allowedHeaders.includes(h[0].toLowerCase())) {
this.headers.delete(h[0]);
}
}
/* TODO(serverhiccups): Once I fix the 'internal response' thing,
these actually need to treat the internal response differently */
} else if (type_ == "opaque" || type_ == "opaqueredirect") {
this.url = "";
this.status = 0;
this.statusText = "";
this.headers = new Headers();
this.body = null;
}
}

this.redirected = redirected_;
}

async arrayBuffer(): Promise<ArrayBuffer> {
if (
this.type == "error" ||
this.type == "opaque" ||
this.type == "opaqueredirect" ||
this.body == null ||
this.body == undefined
) {
return Promise.reject(new Error("Response body is null"));
}
serverhiccups marked this conversation as resolved.
Show resolved Hide resolved
return this.body.arrayBuffer();
}

async blob(): Promise<domTypes.Blob> {
if (
this.type == "error" ||
this.type == "opaque" ||
this.type == "opaqueredirect" ||
this.body == null ||
this.body == undefined
) {
return Promise.reject(new Error("Response body is null"));
}
return this.body.blob();
}

async formData(): Promise<domTypes.FormData> {
if (
this.type == "error" ||
this.type == "opaque" ||
this.type == "opaqueredirect" ||
this.body == null ||
this.body == undefined
) {
return Promise.reject(new Error("Response body is null"));
}
return this.body.formData();
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
async json(): Promise<any> {
if (
this.type == "error" ||
this.type == "opaque" ||
this.type == "opaqueredirect" ||
this.body == null ||
this.body == undefined
) {
return Promise.reject(new Error("Response body is null"));
}
return this.body.json();
}

async text(): Promise<string> {
if (
this.type == "error" ||
this.type == "opaque" ||
this.type == "opaqueredirect" ||
this.body == null ||
this.body == undefined
) {
return Promise.reject(new Error("Response body is null"));
}
return this.body.text();
}

Expand All @@ -306,6 +409,7 @@ export class Response implements domTypes.Response {
}

get bodyUsed(): boolean {
if (this.body === null) return false;
return this.body.bodyUsed;
}

Expand All @@ -332,6 +436,24 @@ export class Response implements domTypes.Response {
this.body
);
}

redirect(url: URL | string, status: number): domTypes.Response {
if (![301, 302, 303, 307, 308].includes(status)) {
throw new RangeError(
"The redirection status must be one of 302, 302, 303, 307 and 308."
serverhiccups marked this conversation as resolved.
Show resolved Hide resolved
);
}
return new Response(
"",
status,
"",
[["Location", typeof url === "string" ? url : url.toString()]],
-1,
false,
null,
"default"
);
}
}

interface FetchResponse {
Expand Down Expand Up @@ -366,6 +488,10 @@ async function sendFetchReq(
return (await sendAsync(dispatch.OP_FETCH, args, zeroCopy)) as FetchResponse;
}

interface NetworkError {
name: string;
message: string;
}
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
/** Fetch a resource from the network. */
export async function fetch(
input: domTypes.Request | URL | string,
Expand Down Expand Up @@ -445,9 +571,15 @@ export async function fetch(
// We're in a redirect status
switch ((init && init.redirect) || "follow") {
case "error":
throw notImplemented();
/* I suspect that deno will probably crash if you try to use that
rid, which suggests to me that Response needs to be refactored */
return new Response("", 0, "", [], -1, false, null, "error");
case "manual":
throw notImplemented();
return new Response("", 0, "", [], -1, false, null, "opaqueredirect");
/* This mode is not part of the standard, but leaving it out would be
annoying to work around. */
case "nofollow":
return response;
case "follow":
default:
let redirectUrl = response.headers.get("Location");
Expand Down
63 changes: 62 additions & 1 deletion cli/js/fetch_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ import {
assert,
assertEquals,
assertStrContains,
assertThrows
assertThrows,
fail
} from "./test_util.ts";

import { Response } from "./fetch.ts";
serverhiccups marked this conversation as resolved.
Show resolved Hide resolved

testPerm({ net: true }, async function fetchConnectionError(): Promise<void> {
let err;
try {
Expand Down Expand Up @@ -360,3 +363,61 @@ testPerm({ net: true }, async function fetchPostBodyTypedArray():Promise<void> {
assertEquals(actual, expected);
});
*/

testPerm({ net: true }, async function fetchWithManualRedirection(): Promise<
void
> {
const response = await fetch("http://localhost:4546/", {
redirect: "manual"
}); // will redirect to http://localhost:4545/
assertEquals(response.status, 0);
assertEquals(response.statusText, "");
assertEquals(response.url, "");
assertEquals(response.type, "opaqueredirect");
try {
await response.text();
fail(
"Reponse.text() didn't throw on a filtered response without a body (type opaqueredirect)"
);
} catch (e) {
return;
}
});

testPerm({ net: true }, async function fetchWithErrorRedirection(): Promise<
void
> {
const response = await fetch("http://localhost:4546/", {
redirect: "error"
}); // will redirect to http://localhost:4545/
assertEquals(response.status, 0);
assertEquals(response.statusText, "");
assertEquals(response.url, "");
assertEquals(response.type, "error");
try {
await response.text();
fail(
"Reponse.text() didn't on a filtered response without a body (type error)"
);
} catch (e) {
return;
}
});

test(function responseRedirect(): void {
const response = new Response(
"example.com/beforeredirect",
200,
"OK",
[["This-Should", "Disappear"]],
-1,
false,
null
);
const redir = response.redirect("example.com/newLocation", 301);
assertEquals(redir.status, 0);
assertEquals(redir.statusText, "");
assertEquals(redir.url, "");
assertEquals(redir.headers.get("Location"), "example.com/newLocation");
assertEquals(redir.type, "default");
});
3 changes: 2 additions & 1 deletion cli/js/test_util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export {
assertNotEquals,
assertStrictEq,
assertStrContains,
unreachable
unreachable,
fail
} from "../../std/testing/asserts.ts";

interface TestPermissions {
Expand Down