-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Improve fetch #853
Improve fetch #853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you - the tests need some clean up - I left some comments there.
@qti3e - please review if you have time and desire.
js/fetch_test.ts
Outdated
const h = new Headers(); | ||
} catch (e) { | ||
assert(false, "Create headers from no parameter failed"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need try catch blocks for successful tests. The test()
function already does that.
You do need to add a name to the function, because that is used in the test output. So, for example, this test should be something like:
test(function newHeadersSuccess() {
new Headers();
});
js/fetch_test.ts
Outdated
} catch (e) { | ||
assert(false, "Create headers from undefined parameter failed"); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
js/fetch_test.ts
Outdated
} catch (e) { | ||
assert(false, "Create headers from empty object failed"); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
} catch (e) { | ||
assertEqual(e.message, "Failed to construct 'Headers': Invalid value"); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just combine all of these into one test case....
js/fetch_test.ts
Outdated
}); | ||
headers.forEach(function(value, key, container) { | ||
assertEqual(headers, container); | ||
assertEqual(value, headerEntriesDict[key]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should run ./tools/format.py too. Use arrow notation for anonymous functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ry Done.
js/fetch_test.ts
Outdated
@@ -93,30 +72,30 @@ for (const name in headerDict) { | |||
headerSeq.push([name, headerDict[name]]); | |||
} | |||
|
|||
test(function() { | |||
test(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs name
js/fetch_test.ts
Outdated
const headers = new Headers(headerSeq); | ||
for (const name in headerDict) { | ||
assertEqual(headers.get(name), String(headerDict[name])); | ||
} | ||
assertEqual(headers.get("length"), null); | ||
}); | ||
|
||
test(function() { | ||
test(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs name
value: init[key] | ||
}); | ||
} | ||
// init is a object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but it's not correct, your logic fails when init
is some number.
try:
new DenoHeader(5);
I suggest is to do this:
if (arguments.length === 0 || init === undefined) {
return;
}
if (init instanceof DenoHeaders) {
...
} else if (Array.isArray(init)) {
...
} else if (Object.prototype.toString.call(init) === "[object Object]") {
// You can use a better is-object test if you have any : )
...
} else {
throw new TypeError("Failed to construct 'Headers': Invalid value");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why type check doesn't work. Maybe it is better to add more check in contructor fucntion.
} | ||
} | ||
|
||
private normalizeParams(name: string, value?: string): string[] { | ||
name = String(name).toLowerCase(); | ||
value = String(value).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trim
removes all HTTP whitespace characters? I don't know about this...
But anyway it's just fine for now
}); | ||
headers.forEach((value, key, container) => { | ||
assertEqual(headers, container); | ||
assertEqual(value, headerEntriesDict[key]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if forEach
does not call the callback at all?
let call_num = 0;
headers.forEach((value, key, container) => {
++call_num;
..
});
assertEqual(call_num, keys.length)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a few comments
@qti3e Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks @ztplz !
- Improve fetch headers (denoland#853) - Add deno.truncate (denoland#805) - Add copyFile/copyFileSync (denoland#863) - Limit depth of output in console.log for nested objects, and add console.dir (denoland#826) - Guess extensions on extension not provided (denoland#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (denoland#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.
- Improve fetch headers (#853) - Add deno.truncate (#805) - Add copyFile/copyFileSync (#863) - Limit depth of output in console.log for nested objects, and add console.dir (#826) - Guess extensions on extension not provided (#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.
#522 Base on spec-whatwg and more test