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

lib: implement fetch's Headers class #37355

Closed
wants to merge 3 commits into from
Closed

Conversation

targos
Copy link
Member

@targos targos commented Feb 13, 2021

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 the fetch API, as defined in https://fetch.spec.whatwg.org/#headers-class

For those who want to help:

# build
./configure --node-builtin-modules-path $(pwd)

# run all WPT tests
./node test/wpt/test-fetch-headers.js

# run a specific test file
./node test/wpt/test-fetch-headers.js headers-record.any.js

@targos targos added the wip Issues and PRs that are still a work in progress. label Feb 13, 2021
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 13, 2021
// https://fetch.spec.whatwg.org/#headers-class
class Headers {
#headerList;
#guard;
Copy link
Member

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.

Copy link
Member

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)

Copy link
Member Author

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);
Copy link
Member

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?

Copy link
Member Author

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.

@ronag
Copy link
Member

ronag commented Feb 20, 2021

@Ethan-Arrowood @delvedor Some overlap with your work?

@Ethan-Arrowood
Copy link
Contributor

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

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

  1. If header’s name is not a no-CORS-safelisted request-header name, then return false.
  2. 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.
Copy link
Contributor

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

Suggested change
// 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([
Copy link
Contributor

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- or Sec- (including being a byte-case-insensitive match for just Proxy- or Sec-).

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) {
Copy link
Contributor

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)) {
Copy link
Contributor

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)) {
Copy link
Contributor

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)) {
Copy link
Contributor

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) {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

@targos
Copy link
Member Author

targos commented Feb 20, 2021

Thanks a lot for the review @Ethan-Arrowood !
I'll get back to this soon to fix the things you mentioned and see how we can split the rest of the work :)

return this.entries();
}

forEach(callback) {
Copy link
Contributor

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.

Comment on lines +353 to +355
[SymbolIterator]() {
return this.entries();
}
Copy link
Contributor

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;
Copy link
Contributor

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:

Suggested change
yield* headers;
yield* new SafeArrayIterator(headers);

@targos
Copy link
Member Author

targos commented Jun 2, 2021

@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 undici-fetch, what do you think?

@Ethan-Arrowood
Copy link
Contributor

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

@Ethan-Arrowood
Copy link
Contributor

@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?

@targos
Copy link
Member Author

targos commented Jun 9, 2021

I think Header would be useful by itself, yes. That's why I started to implement it :)

@jasnell
Copy link
Member

jasnell commented Jun 9, 2021

Big +1... I'd like to see this move forward

@Ethan-Arrowood
Copy link
Contributor

Alright I'll throw something up by eod

@Ethan-Arrowood
Copy link
Contributor

#38986

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants