lib: implement fetch's Headers class #37355

targos · 2021-02-13T15:20:12Z

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

jasnell · 2021-02-14T16:36:29Z

lib/internal/fetch/headers.js

+// https://fetch.spec.whatwg.org/#headers-class
+class Headers {
+  #headerList;
+  #guard;


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.

Likewise on the private functions (like #append)

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.

jasnell · 2021-02-14T16:38:04Z

lib/internal/fetch/headers.js

+
+  // https://fetch.spec.whatwg.org/#dom-headers-get
+  get(name) {
+    name = normalizeName(name);


Would it be better to normalize on header storage rather than on read?

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.

Refs: nodejs#19393

remove private fields

ronag · 2021-02-20T10:58:49Z

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

Ethan-Arrowood · 2021-02-20T13:40:22Z

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

Ethan-Arrowood

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 :)

Ethan-Arrowood · 2021-02-20T18:33:11Z

lib/internal/fetch/headers.js

+
+// https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header
+function isNoCORSSafelistedRequestHeader(name, value) {
+  if (noCORSSafelistedRequestHeaderNames.has(name))


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:

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.

Ethan-Arrowood · 2021-02-20T18:37:16Z

lib/internal/fetch/headers.js

+      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.


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.

Ethan-Arrowood · 2021-02-20T18:41:45Z

lib/internal/fetch/headers.js

+}
+
+// https://fetch.spec.whatwg.org/#forbidden-header-name
+const forbiddenHeaderNames = new SafeSet([


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.

Ethan-Arrowood · 2021-02-20T18:47:03Z

lib/internal/fetch/headers.js

+  }
+
+  // https://fetch.spec.whatwg.org/#concept-header-list-get-structured-header
+  getStructuredFieldValue(name, type) {


Missing:

Assert: type is one of "dictionary", "list", or "item".

Ethan-Arrowood · 2021-02-20T18:55:49Z

lib/internal/fetch/headers.js

+
+  if (headers[kGuard] === 'immutable') {
+    throw new TypeError('this Headers object is immutable');
+  } else if (headers[kGuard] === 'request' && forbiddenHeaderNames.has(lowerName)) {


Referencing earlier comment, this check misses out on the Proxy- and Sec- forbidden headers

Ethan-Arrowood · 2021-02-20T19:01:17Z

lib/internal/fetch/headers.js

+    if (this[kGuard] === 'immutable') {
+      throw new TypeError('this Headers object is immutable');
+    }
+    if (this[kGuard] === 'request' && forbiddenHeaderNames.has(lowerName)) {


Referencing previous comment - this check misses Proxy- and Sec-

Ethan-Arrowood · 2021-02-20T19:02:59Z

lib/internal/fetch/headers.js

+    if (this[kGuard] === 'immutable') {
+      throw new TypeError('this Headers object is immutable');
+    }
+    if (this[kGuard] === 'request' && forbiddenHeaderNames.has(lowerName)) {


Same as other forbiddenHeaderNames comments

Ethan-Arrowood · 2021-02-20T19:04:47Z

lib/internal/fetch/headers.js

+    return this.entries();
+  }
+
+  forEach(callback) {


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?

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.

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 · 2021-02-20T19:15:36Z

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 :)

ExE-Boss · 2021-03-26T06:45:18Z

lib/internal/fetch/headers.js

+    return this.entries();
+  }
+
+  forEach(callback) {


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.

ExE-Boss · 2021-03-26T06:48:23Z

lib/internal/fetch/headers.js

+  [SymbolIterator]() {
+    return this.entries();
+  }


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, });

ExE-Boss · 2021-03-26T06:57:13Z

lib/internal/fetch/headers.js

+
+  *entries() {
+    const headers = this[kHeaderList].sortAndCombine();
+    yield* headers;


headers is an array, and yield* performs an unsafe iteration:

Suggested change

yield* headers;

yield* new SafeArrayIterator(headers);

targos · 2021-06-02T10:19:07Z

@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 · 2021-06-04T15:16:01Z

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 · 2021-06-08T21:33:52Z

@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 · 2021-06-09T09:52:24Z

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

jasnell · 2021-06-09T12:38:03Z

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

Ethan-Arrowood · 2021-06-09T14:21:40Z

Alright I'll throw something up by eod

Ethan-Arrowood · 2021-06-10T03:14:57Z

#38986

targos added the wip Issues and PRs that are still a work in progress. label Feb 13, 2021

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 13, 2021

jasnell reviewed Feb 14, 2021

View reviewed changes

targos added 3 commits February 16, 2021 16:27

lib: implement fetch's Headers class

47e0894

Refs: nodejs#19393

fixup! lib: implement fetch's Headers class

a6911ad

fixup! lib: implement fetch's Headers class

4ba1b93

remove private fields

targos force-pushed the fetch-headers branch from 5fb7ba2 to 4ba1b93 Compare February 16, 2021 16:49

Ethan-Arrowood reviewed Feb 20, 2021

View reviewed changes

ExE-Boss reviewed Mar 26, 2021

View reviewed changes

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

ronag mentioned this pull request Jun 4, 2021

Potential breaking change on v14.17.0 #38922

Closed

targos closed this Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: implement fetch's Headers class #37355

lib: implement fetch's Headers class #37355

targos commented Feb 13, 2021

jasnell Feb 14, 2021

jasnell Feb 14, 2021

targos Feb 16, 2021

jasnell Feb 14, 2021

targos Feb 14, 2021

ronag commented Feb 20, 2021

Ethan-Arrowood commented Feb 20, 2021

Ethan-Arrowood left a comment

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

Ethan-Arrowood Feb 20, 2021

targos Feb 20, 2021

ExE-Boss Mar 26, 2021

targos commented Feb 20, 2021

ExE-Boss Mar 26, 2021

ExE-Boss Mar 26, 2021

ExE-Boss Mar 26, 2021

targos commented Jun 2, 2021

Ethan-Arrowood commented Jun 4, 2021

Ethan-Arrowood commented Jun 8, 2021

targos commented Jun 9, 2021

jasnell commented Jun 9, 2021

Ethan-Arrowood commented Jun 9, 2021

Ethan-Arrowood commented Jun 10, 2021

	if (noCORSSafelistedRequestHeaderNames.has(name))
	if (!noCORSSafelistedRequestHeaderNames.has(name))

	// 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.

lib: implement fetch's Headers class #37355

lib: implement fetch's Headers class #37355

Conversation

targos commented Feb 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag commented Feb 20, 2021

Ethan-Arrowood commented Feb 20, 2021

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Jun 2, 2021

Ethan-Arrowood commented Jun 4, 2021

Ethan-Arrowood commented Jun 8, 2021

targos commented Jun 9, 2021

jasnell commented Jun 9, 2021

Ethan-Arrowood commented Jun 9, 2021

Ethan-Arrowood commented Jun 10, 2021