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
Reduce number of requests in header-values-normalize #4583
Conversation
See discussion in #4525.
Notifying @jdm and @youennf. (Learn how reviewing works.) |
if(fail) { | ||
promise_rejects(t, new TypeError(), fetch("about:blank", { headers: {"val1": val1} })) | ||
promise_rejects(t, new TypeError(), fetch("about:blank", { headers: {"val2": val2} })) | ||
return promise_rejects(t, new TypeError(), fetch("about:blank", { headers: {"val3": val3} })) |
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'm not sure about the above three lines being the correct way to do this. I asked in #whatwg but nobody was around to answer.
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 probably want that to be return Promise.all([promise_rejects(...), promise_rejects(...), promise_rejects(...)])
?
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.
Does that work? I cannot really find any documentation about how this is supposed to work. @jgraham?
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.
Yeah, the callback to promise_test needs to return a single promise that encompasses all asynchronous behaviour. Promise.all is what you want here.
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.
Thanks, fixed.
Firefox (nightly channel)Testing web-platform-tests at revision 6180f11 All results/fetch/api/headers/header-values-normalize.html
|
Chrome (unstable channel)Testing web-platform-tests at revision 6180f11 All results/fetch/api/headers/header-values-normalize.html
|
Yep! |
Looks good to me. |
See discussion in #4525.