-
Notifications
You must be signed in to change notification settings - Fork 31k
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
lib: implement fetch's Headers class #37355
Conversation
lib/internal/fetch/headers.js
Outdated
// https://fetch.spec.whatwg.org/#headers-class | ||
class Headers { | ||
#headerList; | ||
#guard; |
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.
For the time being, private fields still have a massive performance cost, and I'm afraid that the rest of this class is already going to suffer some performance issues. Likely best to use hidden symbols for now.
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.
Likewise on the private functions (like #append
)
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'll change the private properties and functions but for the rest of the changes, can we start by being close to the spec, make WPT tests pass and only then (in subsequent PRs) care about the performance?
This is a new API and it won't be used in an existing core module so even if it can be faster, nobody will directly suffer from bad perf.
|
||
// https://fetch.spec.whatwg.org/#dom-headers-get | ||
get(name) { | ||
name = normalizeName(name); |
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.
Would it be better to normalize on header storage rather than on read?
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 called the function normalizeName
to be consistent with nomalizeValue
but it just does a check for:
If name is not a name, then throw a TypeError.
5fb7ba2
to
4ba1b93
Compare
@Ethan-Arrowood @delvedor Some overlap with your work? |
This looks good - I'll find some time this weekend to give it a proper review and compare it to what I have in undici-fetch |
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.
Great work! I realized it is still WIP but I did a side-by-side review with the spec anyways. I skipped over any todo parts but still found some inconsistencies. Additionally, if you'd like a hand implementing some of these bits let me know! Happy to contribute :)
|
||
// https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header | ||
function isNoCORSSafelistedRequestHeader(name, value) { | ||
if (noCORSSafelistedRequestHeaderNames.has(name)) |
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.
if (noCORSSafelistedRequestHeaderNames.has(name)) | |
if (!noCORSSafelistedRequestHeaderNames.has(name)) |
Based on spec I believe this needs to be a not operation
To determine whether a header header is a no-CORS-safelisted request-header, run these steps:
- If header’s name is not a no-CORS-safelisted request-header name, then return false.
- Return whether header is a CORS-safelisted request-header.
const mimeType = parseMIMEType(value); | ||
if (mimeType === null) | ||
return false; | ||
// If mimeType’s essence is not "application/x-www-form-urlencoded", "multipart/form-data", or "text/plain", then return false. |
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.
Might be worth indicating that this part of the function is incomplete
// If mimeType’s essence is not "application/x-www-form-urlencoded", "multipart/form-data", or "text/plain", then return false. | |
// TODO: If mimeType’s essence is not "application/x-www-form-urlencoded", "multipart/form-data", or "text/plain", then return false. |
} | ||
|
||
// https://fetch.spec.whatwg.org/#forbidden-header-name | ||
const forbiddenHeaderNames = new SafeSet([ |
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.
this list (and I believe the later checks) are missing:
or a header name that starts with a byte-case-insensitive match for
Proxy-
orSec-
(including being a byte-case-insensitive match for justProxy-
orSec-
).
But I may be wrong I haven't read through all of the other code yet so I'll circle back on this comment later.
} | ||
|
||
// https://fetch.spec.whatwg.org/#concept-header-list-get-structured-header | ||
getStructuredFieldValue(name, type) { |
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.
Missing:
Assert: type is one of "dictionary", "list", or "item".
|
||
if (headers[kGuard] === 'immutable') { | ||
throw new TypeError('this Headers object is immutable'); | ||
} else if (headers[kGuard] === 'request' && forbiddenHeaderNames.has(lowerName)) { |
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.
Referencing earlier comment, this check misses out on the Proxy-
and Sec-
forbidden headers
if (this[kGuard] === 'immutable') { | ||
throw new TypeError('this Headers object is immutable'); | ||
} | ||
if (this[kGuard] === 'request' && forbiddenHeaderNames.has(lowerName)) { |
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.
Referencing previous comment - this check misses Proxy-
and Sec-
if (this[kGuard] === 'immutable') { | ||
throw new TypeError('this Headers object is immutable'); | ||
} | ||
if (this[kGuard] === 'request' && forbiddenHeaderNames.has(lowerName)) { |
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.
Same as other forbiddenHeaderNames comments
return this.entries(); | ||
} | ||
|
||
forEach(callback) { |
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.
Where does this come from? is this apart of being an iterable
? If so is there a better way to indicate this such as extending from some parent class?
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. This is one of the many things that I don't understand about the spec. I don't know if it's supposed to be here because of iterable
or something else, but I added the method to make some of the web platform tests pass.
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.
Yes, it’s here because of iterable
: https://heycam.github.io/webidl/#es-iterable
Also, for better spec compliancy, it should be:
forEach(callback, thisArg = undefined) {
// 1. Let esValue be ? ToObject(this value).
// 3. If esValue does not implement definition, then throw a TypeError.
// Note that using private properties would take care of this check for us:
const headerList = this[kHeaderList];
if (!headerList) {
throw new ERR_INVALID_ARG_TYPE('this', 'Headers', this);
}
// 4. Let idlCallback be callback, converted to a Function.
validateFunction(callback, 'callback');
// 5. Let idlObject be the IDL interface type value that represents a reference to esValue.
// 6. Let pairs be idlObject's list of value pairs to iterate over.
const pairs = headerList.sortAndCombine();
// 7. Let i be 0.
// 8. While i < pairs's size:
for (let i = 0; i < pairs.length; i++) {
// 1. Let pair be pairs[i].
const pair = pairs[i];
// 2. Invoke idlCallback with « pair's value, pair's key, idlObject » and with thisArg as the callback this value.
FunctionPrototypeCall(callback, thisArg, pair[1], pair[0], this);
// 3. Set pairs to idlObject's current list of value pairs to iterate over. (It might have changed.)
// pairs = headerList.sortAndCombine();
// 4. Set i to i + 1.
}
}
This also avoids unsafe iteration over the headers
array.
Thanks a lot for the review @Ethan-Arrowood ! |
return this.entries(); | ||
} | ||
|
||
forEach(callback) { |
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.
Yes, it’s here because of iterable
: https://heycam.github.io/webidl/#es-iterable
Also, for better spec compliancy, it should be:
forEach(callback, thisArg = undefined) {
// 1. Let esValue be ? ToObject(this value).
// 3. If esValue does not implement definition, then throw a TypeError.
// Note that using private properties would take care of this check for us:
const headerList = this[kHeaderList];
if (!headerList) {
throw new ERR_INVALID_ARG_TYPE('this', 'Headers', this);
}
// 4. Let idlCallback be callback, converted to a Function.
validateFunction(callback, 'callback');
// 5. Let idlObject be the IDL interface type value that represents a reference to esValue.
// 6. Let pairs be idlObject's list of value pairs to iterate over.
const pairs = headerList.sortAndCombine();
// 7. Let i be 0.
// 8. While i < pairs's size:
for (let i = 0; i < pairs.length; i++) {
// 1. Let pair be pairs[i].
const pair = pairs[i];
// 2. Invoke idlCallback with « pair's value, pair's key, idlObject » and with thisArg as the callback this value.
FunctionPrototypeCall(callback, thisArg, pair[1], pair[0], this);
// 3. Set pairs to idlObject's current list of value pairs to iterate over. (It might have changed.)
// pairs = headerList.sortAndCombine();
// 4. Set i to i + 1.
}
}
This also avoids unsafe iteration over the headers
array.
[SymbolIterator]() { | ||
return this.entries(); | ||
} |
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.
https://heycam.github.io/webidl/#es-iterable specifies that the Symbol.iterator
property is an alias for the entries
property, so this should use:
ObjectDefineProperty(Headers.prototype, Symbol.iterator, {
configurable: true,
enumerable: false,
writable: true,
value: Headers.prototype.entries,
});
|
||
*entries() { | ||
const headers = this[kHeaderList].sortAndCombine(); | ||
yield* headers; |
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.
headers
is an array, and yield*
performs an unsafe iteration:
yield* headers; | |
yield* new SafeArrayIterator(headers); |
@Ethan-Arrowood I don't know when/if I will have time to come back to this. I feel like it would be a better use of everyone's time to keep the focus on |
Sounds good to me @targos I have some exciting stuff landing soon: Ethan-Arrowood/undici-fetch#48 Just have a couple more pieces to work on. Most likely will get to it this weekend |
@targos do you think it would be valuable for me to try and land my Header class implementation in Node core without the rest of the fetch stuff? |
I think |
Big +1... I'd like to see this move forward |
Alright I'll throw something up by eod |
Refs: #19393
This is not ready yet, but I'm already opening the PR so people know I'm working on it and hopefully to get some help.
It aims to implement the
Headers
class, required for thefetch
API, as defined in https://fetch.spec.whatwg.org/#headers-classFor those who want to help: