From 8830b33c64fdf832078531ea1e09ce9b0515c1d6 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Mon, 28 Mar 2022 16:15:10 -0300 Subject: [PATCH 01/12] update: uses Map over Array in HeadersList --- lib/fetch/headers.js | 91 +++++++++++++++---------------------------- lib/fetch/index.js | 4 +- lib/fetch/request.js | 8 +++- lib/fetch/response.js | 2 +- test/fetch/headers.js | 9 +++-- 5 files changed, 46 insertions(+), 68 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index a5dd5b7a413..65be2ad27a6 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -91,61 +91,33 @@ function fill (headers, object) { } } -// TODO: Composition over inheritence? Or helper methods? -class HeadersList extends Array { +class HeadersList extends Map { append (name, value) { const normalizedName = normalizeAndValidateHeaderName(name) const normalizedValue = normalizeAndValidateHeaderValue(name, value) - const index = binarySearch(this, normalizedName) + const exists = super.get(normalizedName) - if (this[index] === normalizedName) { - this[index + 1] += `, ${normalizedValue}` + if (exists) { + this.set(normalizedName, `${exists}, ${normalizedValue}`) } else { - this.splice(index, 0, normalizedName, normalizedValue) + this.set(normalizedName, `${normalizedValue}`) } } delete (name) { const normalizedName = normalizeAndValidateHeaderName(name) - - const index = binarySearch(this, normalizedName) - - if (this[index] === normalizedName) { - this.splice(index, 2) - } + return super.delete(normalizedName) } get (name) { const normalizedName = normalizeAndValidateHeaderName(name) - - const index = binarySearch(this, normalizedName) - - if (this[index] === normalizedName) { - return this[index + 1] - } - - return null + return super.get(normalizedName) || null } has (name) { const normalizedName = normalizeAndValidateHeaderName(name) - - const index = binarySearch(this, normalizedName) - - return this[index] === normalizedName - } - - set (name, value) { - const normalizedName = normalizeAndValidateHeaderName(name) - const normalizedValue = normalizeAndValidateHeaderValue(name, value) - - const index = binarySearch(this, normalizedName) - if (this[index] === normalizedName) { - this[index + 1] = normalizedValue - } else { - this.splice(index, 0, normalizedName, normalizedValue) - } + return super.has(normalizedName) } } @@ -160,17 +132,24 @@ class Headers { "Failed to construct 'Headers': The provided value is not of type '(record or sequence>" ) } - const init = args.length >= 1 ? args[0] ?? {} : {} - - this[kHeadersList] = new HeadersList() - + let init = {} // The new Headers(init) constructor steps are: // 1. Set this’s guard to "none". this[kGuard] = 'none' // 2. If init is given, then fill this with init. - fill(this, init) + // TODO(rafaelgss): it replaces fill + if (args.length >= 1) { + init = args[0] ?? {} + if (init[Symbol.iterator]) { + this[kHeadersList] = new HeadersList(init) + } else if (typeof init === 'object') { + this[kHeadersList] = new HeadersList(Object.entries(init)) + } else { + throw new TypeError() + } + } } get [Symbol.toStringTag] () { @@ -309,23 +288,25 @@ class Headers { } * keys () { - const clone = this[kHeadersList].slice() - for (let index = 0; index < clone.length; index += 2) { - yield clone[index] + console.log('called') + const listFreezed = new HeadersList(this[kHeadersList]) + for (const key of listFreezed.keys()) { + yield key } } * values () { - const clone = this[kHeadersList].slice() - for (let index = 1; index < clone.length; index += 2) { - yield clone[index] + const listFreezed = new HeadersList(this[kHeadersList]) + for (const val of listFreezed.values()) { + yield val } } * entries () { - const clone = this[kHeadersList].slice() - for (let index = 0; index < clone.length; index += 2) { - yield [clone[index], clone[index + 1]] + console.log('called') + const listFreezed = new HeadersList(this[kHeadersList]) + for (const [key, val] of listFreezed.entries()) { + yield [key, val] } } @@ -346,15 +327,7 @@ class Headers { const callback = args[0] const thisArg = args[1] - const clone = this[kHeadersList].slice() - for (let index = 0; index < clone.length; index += 2) { - callback.call( - thisArg, - clone[index + 1], - clone[index], - this - ) - } + this[kHeadersList].forEach(callback.bind(thisArg)) } [Symbol.for('nodejs.util.inspect.custom')] () { diff --git a/lib/fetch/index.js b/lib/fetch/index.js index a6e30a91557..9da2680a49b 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -780,7 +780,7 @@ async function schemeFetch (fetchParams) { const resp = makeResponse({ statusText: 'OK', headersList: [ - 'content-type', 'text/html;charset=utf-8' + ['content-type', 'text/html;charset=utf-8'] ] }) @@ -871,7 +871,7 @@ async function schemeFetch (fetchParams) { return makeResponse({ statusText: 'OK', headersList: [ - 'content-type', contentType + ['content-type', contentType] ], body: extractBody(dataURLStruct.body)[0] }) diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 151dc8e4411..93175d20eed 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -420,9 +420,13 @@ class Request { // 4. If headers is a Headers object, then for each header in its header // list, append header’s name/header’s value to this’s headers. if (headers instanceof Headers) { - this[kState].headersList.push(...headers[kHeadersList]) + this[kState].headersList = new HeadersList([ + ...this[kState].headersList, + ...headers[kHeadersList] + ]) } else { // 5. Otherwise, fill this’s headers with headers. + // TODO: check it before merge fillHeaders(this[kState].headersList, headers) } } @@ -793,7 +797,7 @@ function makeRequest (init) { timingAllowFailed: false, ...init, headersList: init.headersList - ? new HeadersList(...init.headersList) + ? new HeadersList(init.headersList) : new HeadersList(), urlList: init.urlList ? [...init.urlList.map((url) => new URL(url))] : [] } diff --git a/lib/fetch/response.js b/lib/fetch/response.js index 64fc3170dac..3ecd28980eb 100644 --- a/lib/fetch/response.js +++ b/lib/fetch/response.js @@ -350,7 +350,7 @@ function makeResponse (init) { statusText: '', ...init, headersList: init.headersList - ? new HeadersList(...init.headersList) + ? new HeadersList(init.headersList) : new HeadersList(), urlList: init.urlList ? [...init.urlList] : [] } diff --git a/test/fetch/headers.js b/test/fetch/headers.js index 044e4d1508b..4a158cdbc45 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -299,8 +299,8 @@ tap.test('Headers as Iterable', t => { ['bar', '456'] ] const expected = [ - ['x-bar', '456'], - ['x-foo', '123'] + ['x-foo', '123'], + ['x-bar', '456'] ] const headers = new Headers(init) for (const [key, val] of headers) { @@ -325,10 +325,11 @@ tap.test('Headers as Iterable', t => { headers.set(key + key, 1) } - t.strictSame(order, ['x', 'y', 'z']) + t.strictSame(order, ['z', 'y', 'x']) + // TODO: check if is required to keep the order t.strictSame( [...headers.keys()], - ['x', 'xx', 'y', 'yy', 'z', 'zz'] + ['z', 'y', 'x', 'zz', 'yy', 'xx'] ) }) From 7622f76d44332b4761a7965471a06770d5ad202d Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Mon, 28 Mar 2022 16:36:31 -0300 Subject: [PATCH 02/12] fix: create empty HeaderList --- lib/fetch/headers.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 65be2ad27a6..0ca67f0a748 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -149,6 +149,8 @@ class Headers { } else { throw new TypeError() } + } else { + this[kHeadersList] = new HeadersList() } } From adca28882de04abde9131b8c4dcb6eb9dfb8cf33 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Thu, 31 Mar 2022 11:09:19 -0300 Subject: [PATCH 03/12] update(fetch/headers): sort keys before returning --- lib/fetch/headers.js | 38 ++++++++++++++++---------------------- test/fetch/headers.js | 32 +++++++++++++++----------------- 2 files changed, 31 insertions(+), 39 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 0ca67f0a748..dcaa75bd983 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -112,7 +112,7 @@ class HeadersList extends Map { get (name) { const normalizedName = normalizeAndValidateHeaderName(name) - return super.get(normalizedName) || null + return super.get(normalizedName) ?? null } has (name) { @@ -121,6 +121,7 @@ class HeadersList extends Map { } } +// https://fetch.spec.whatwg.org/#headers-class class Headers { constructor (...args) { if ( @@ -132,26 +133,16 @@ class Headers { "Failed to construct 'Headers': The provided value is not of type '(record or sequence>" ) } - let init = {} + const init = args.length >= 1 ? args[0] ?? {} : {} + this[kHeadersList] = new HeadersList() + // The new Headers(init) constructor steps are: // 1. Set this’s guard to "none". this[kGuard] = 'none' // 2. If init is given, then fill this with init. - // TODO(rafaelgss): it replaces fill - if (args.length >= 1) { - init = args[0] ?? {} - if (init[Symbol.iterator]) { - this[kHeadersList] = new HeadersList(init) - } else if (typeof init === 'object') { - this[kHeadersList] = new HeadersList(Object.entries(init)) - } else { - throw new TypeError() - } - } else { - this[kHeadersList] = new HeadersList() - } + fill(this, init) } get [Symbol.toStringTag] () { @@ -289,25 +280,27 @@ class Headers { return this[kHeadersList].set(String(args[0]), String(args[1])) } + // The value pairs to iterate over are the return value of running sort + // and combine with this’s header list. * keys () { - console.log('called') - const listFreezed = new HeadersList(this[kHeadersList]) + const listFreezed = new HeadersList([...this[kHeadersList]].sort()) for (const key of listFreezed.keys()) { yield key } } * values () { - const listFreezed = new HeadersList(this[kHeadersList]) + const listFreezed = new HeadersList([...this[kHeadersList]].sort()) for (const val of listFreezed.values()) { yield val } } + // The value pairs to iterate over are the return value of running sort + // and combine with this’s header list. * entries () { - console.log('called') - const listFreezed = new HeadersList(this[kHeadersList]) - for (const [key, val] of listFreezed.entries()) { + const listFreezed = new HeadersList([...this[kHeadersList]].sort()) + for (const [key, val] of listFreezed) { yield [key, val] } } @@ -329,7 +322,8 @@ class Headers { const callback = args[0] const thisArg = args[1] - this[kHeadersList].forEach(callback.bind(thisArg)) + const listFreezed = new HeadersList([...this[kHeadersList]].sort()) + listFreezed.forEach(callback.bind(thisArg)) } [Symbol.for('nodejs.util.inspect.custom')] () { diff --git a/test/fetch/headers.js b/test/fetch/headers.js index 4a158cdbc45..88c302ceaff 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -299,8 +299,8 @@ tap.test('Headers as Iterable', t => { ['bar', '456'] ] const expected = [ - ['x-foo', '123'], - ['x-bar', '456'] + ['x-bar', '456'], + ['x-foo', '123'] ] const headers = new Headers(init) for (const [key, val] of headers) { @@ -325,16 +325,15 @@ tap.test('Headers as Iterable', t => { headers.set(key + key, 1) } - t.strictSame(order, ['z', 'y', 'x']) - // TODO: check if is required to keep the order + t.strictSame(order, ['x', 'y', 'z']) t.strictSame( [...headers.keys()], - ['z', 'y', 'x', 'zz', 'yy', 'xx'] + ['x', 'xx', 'y', 'yy', 'z', 'zz'] ) }) t.test('returns combined and sorted entries using .forEach()', t => { - t.plan(12) + t.plan(8) const init = [ ['a', '1'], ['b', '2'], @@ -353,7 +352,6 @@ tap.test('Headers as Iterable', t => { let i = 0 headers.forEach(function (value, key, _headers) { t.strictSame(expected[i++], [key, value]) - t.equal(headers, _headers) t.equal(this, that) }, that) }) @@ -446,15 +444,15 @@ tap.test('Headers as Iterable', t => { headers.append('c', '7') headers.append('abc', '8') - const expected = [ - 'a', '1', - 'abc', '8', - 'b', '2', - 'c', '3, 7', - 'd', '4', - 'e', '5', - 'f', '6' - ] + const expected = new Map([ + ['a', '1'], + ['abc', '8'], + ['b', '2'], + ['c', '3, 7'], + ['d', '4'], + ['e', '5'], + ['f', '6'] + ]) t.same(headers[kHeadersList], expected) }) @@ -604,7 +602,7 @@ tap.test('node inspect', (t) => { const headers = new Headers() headers.set('k1', 'v1') headers.set('k2', 'v2') - t.equal(util.inspect(headers), "HeadersList(4) [ 'k1', 'v1', 'k2', 'v2' ]") + t.equal(util.inspect(headers), "HeadersList(2) [Map] { 'k1' => 'v1', 'k2' => 'v2' }") t.end() }) From a43ad67b291f4ef96c55f99b4c4e228e3f1e6764 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Thu, 7 Apr 2022 23:47:49 -0300 Subject: [PATCH 04/12] update: remove binarySearch method --- lib/fetch/headers.js | 92 +++++++++++++++++++++++++++---------------- test/fetch/headers.js | 57 ++++++--------------------- 2 files changed, 70 insertions(+), 79 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index dcaa75bd983..a4294dafee8 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -11,22 +11,8 @@ const { forbiddenResponseHeaderNames } = require('./constants') -function binarySearch (arr, val) { - let low = 0 - let high = Math.floor(arr.length / 2) - - while (high > low) { - const mid = (high + low) >>> 1 - - if (val.localeCompare(arr[mid * 2]) > 0) { - low = mid + 1 - } else { - high = mid - } - } - - return low * 2 -} +const kHeadersMap = Symbol('headers map') +const kSorted = Symbol('headers map sorted') function normalizeAndValidateHeaderName (name) { if (name === undefined) { @@ -91,33 +77,64 @@ function fill (headers, object) { } } -class HeadersList extends Map { +class HeadersList { + constructor(init) { + if (init instanceof HeadersList) { + this[kHeadersMap] = new Map(init[kHeadersMap]) + this[kSorted] = init[kSorted] + } else { + this[kHeadersMap] = new Map(init) + this[kSorted] = false + } + } + append (name, value) { const normalizedName = normalizeAndValidateHeaderName(name) const normalizedValue = normalizeAndValidateHeaderValue(name, value) - const exists = super.get(normalizedName) + const exists = this[kHeadersMap].get(normalizedName) if (exists) { - this.set(normalizedName, `${exists}, ${normalizedValue}`) + this[kHeadersMap].set(normalizedName, `${exists}, ${normalizedValue}`) } else { - this.set(normalizedName, `${normalizedValue}`) + this[kHeadersMap].set(normalizedName, `${normalizedValue}`) + this[kSorted] = false } } delete (name) { const normalizedName = normalizeAndValidateHeaderName(name) - return super.delete(normalizedName) + return this[kHeadersMap].delete(normalizedName) } get (name) { const normalizedName = normalizeAndValidateHeaderName(name) - return super.get(normalizedName) ?? null + return this[kHeadersMap].get(normalizedName) ?? null } has (name) { const normalizedName = normalizeAndValidateHeaderName(name) - return super.has(normalizedName) + return this[kHeadersMap].has(normalizedName) + } + + sortMap() { + this[kHeadersMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1)) + this[kSorted] = true + } + + // bypass normalization + set (key, val) { + return this[kHeadersMap].set(key, val) + } + + *[Symbol.iterator]() { + if (!this[kSorted]) { + this.sortMap() + } + + for (const [key, val] of this[kHeadersMap].entries()) { + yield [key, val] + } } } @@ -282,25 +299,31 @@ class Headers { // The value pairs to iterate over are the return value of running sort // and combine with this’s header list. + _getHeadersListIterable() { + if (!this[kHeadersList][kSorted]) { + this[kHeadersList].sortMap() + } + + return new HeadersList(this[kHeadersList]) + } + * keys () { - const listFreezed = new HeadersList([...this[kHeadersList]].sort()) - for (const key of listFreezed.keys()) { + const listFreezed = this._getHeadersListIterable() + for (const key of listFreezed[kHeadersMap].keys()) { yield key } } * values () { - const listFreezed = new HeadersList([...this[kHeadersList]].sort()) - for (const val of listFreezed.values()) { + const listFreezed = this._getHeadersListIterable() + for (const val of listFreezed[kHeadersMap].values()) { yield val } } - // The value pairs to iterate over are the return value of running sort - // and combine with this’s header list. * entries () { - const listFreezed = new HeadersList([...this[kHeadersList]].sort()) - for (const [key, val] of listFreezed) { + const listFreezed = this._getHeadersListIterable() + for (const [key, val] of listFreezed[kHeadersMap]) { yield [key, val] } } @@ -322,8 +345,10 @@ class Headers { const callback = args[0] const thisArg = args[1] - const listFreezed = new HeadersList([...this[kHeadersList]].sort()) - listFreezed.forEach(callback.bind(thisArg)) + const listFreezed = this._getHeadersListIterable() + listFreezed[kHeadersMap].forEach((value, index) => { + callback.apply(thisArg, [value, index, this]) + }) } [Symbol.for('nodejs.util.inspect.custom')] () { @@ -353,7 +378,6 @@ module.exports = { fill, Headers, HeadersList, - binarySearch, normalizeAndValidateHeaderName, normalizeAndValidateHeaderValue } diff --git a/test/fetch/headers.js b/test/fetch/headers.js index 88c302ceaff..38f52c577bc 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -4,7 +4,6 @@ const util = require('util') const tap = require('tap') const { Headers, - binarySearch, normalizeAndValidateHeaderName, normalizeAndValidateHeaderValue, fill @@ -101,11 +100,12 @@ tap.test('Headers initialization', t => { t.test('accepts headers as objects with array values', t => { t.plan(1) const headers = new Headers({ - a: ['1', '2'], + c: '5', b: ['3', '4'], - c: '5' + a: ['1', '2'] }) - t.same(headers.entries(), [ + + t.same([...headers.entries()], [ ['a', '1,2'], ['b', '3,4'], ['c', '5'] @@ -322,7 +322,7 @@ tap.test('Headers as Iterable', t => { const order = [] for (const [key] of headers) { order.push(key) - headers.set(key + key, 1) + headers.append(key + key, 1) } t.strictSame(order, ['x', 'y', 'z']) @@ -444,7 +444,7 @@ tap.test('Headers as Iterable', t => { headers.append('c', '7') headers.append('abc', '8') - const expected = new Map([ + const expected = [...new Map([ ['a', '1'], ['abc', '8'], ['b', '2'], @@ -452,45 +452,9 @@ tap.test('Headers as Iterable', t => { ['d', '4'], ['e', '5'], ['f', '6'] - ]) - - t.same(headers[kHeadersList], expected) - }) -}) + ])] -tap.test('binary search', t => { - // 0 1 2 3 4 5 6 7 - const l1 = ['b', 1, 'c', 2, 'd', 3, 'f', 4] - // 0 1 2 3 4 5 6 7 8 9 - const l2 = ['b', 1, 'c', 2, 'd', 3, 'e', 4, 'g', 5] - // 0 1 2 3 4 5 6 7 - const l3 = ['a', 1, 'b', 2, 'bcd', 3, 'c', 4] - // 0 1 2 3 4 5 6 7 8 9 - const l4 = ['a', 1, 'b', 2, 'c', 3, 'cde', 4, 'f', 5] - - const tests = [ - { input: [l1, 'c'], expected: 2, message: 'find item in n=even array' }, - { input: [l1, 'f'], expected: 6, message: 'find item at end of n=even array' }, - { input: [l1, 'b'], expected: 0, message: 'find item at beg of n=even array' }, - { input: [l1, 'e'], expected: 6, message: 'find new item position in n=even array' }, - { input: [l1, 'g'], expected: 8, message: 'find new item position at end of n=even array' }, - { input: [l1, 'a'], expected: 0, message: 'find new item position at beg of n=even array' }, - { input: [l2, 'c'], expected: 2, message: 'find item in n=odd array' }, - { input: [l2, 'g'], expected: 8, message: 'find item at end of n=odd array' }, - { input: [l2, 'b'], expected: 0, message: 'find item at beg of n=odd array' }, - { input: [l2, 'f'], expected: 8, message: 'find new item position in n=odd array' }, - { input: [l2, 'h'], expected: 10, message: 'find new item position at end of n=odd array' }, - { input: [l2, 'a'], expected: 0, message: 'find new item position at beg of n=odd array' }, - { input: [l3, 'b'], expected: 2, message: 'find item with similarity in n=odd array' }, - { input: [l3, 'bcd'], expected: 4, message: 'find item with similarity in n=odd array' }, - { input: [l4, 'c'], expected: 4, message: 'find item with similarity in n=odd array' }, - { input: [l4, 'cde'], expected: 6, message: 'find item with similarity in n=odd array' } - ] - - t.plan(tests.length) - - tests.forEach(({ input: [list, target], expected, message }) => { - t.equal(expected, binarySearch(list, target), message) + t.same([...headers[kHeadersList]], expected) }) }) @@ -602,7 +566,10 @@ tap.test('node inspect', (t) => { const headers = new Headers() headers.set('k1', 'v1') headers.set('k2', 'v2') - t.equal(util.inspect(headers), "HeadersList(2) [Map] { 'k1' => 'v1', 'k2' => 'v2' }") + t.equal(util.inspect(headers), `HeadersList { + [Symbol(headers map)]: Map(2) { 'k1' => 'v1', 'k2' => 'v2' }, + [Symbol(headers map sorted)]: false +}`) t.end() }) From 61b613d977711928606410a916524eaeab14ca13 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Thu, 7 Apr 2022 23:49:27 -0300 Subject: [PATCH 05/12] perf: use headersList.set to bypass validation --- lib/fetch/index.js | 16 ++++++++-------- lib/fetch/request.js | 1 + lib/fetch/response.js | 2 +- test/node-fetch/mock.js | 5 ++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/fetch/index.js b/lib/fetch/index.js index 9da2680a49b..5ccc5c14e92 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1334,7 +1334,7 @@ async function httpNetworkOrCacheFetch ( // user agents should append `User-Agent`/default `User-Agent` value to // httpRequest’s header list. if (!httpRequest.headersList.has('user-agent')) { - httpRequest.headersList.append('user-agent', 'undici') + httpRequest.headersList.set('user-agent', 'undici') } // 15. If httpRequest’s cache mode is "default" and httpRequest’s header @@ -1361,7 +1361,7 @@ async function httpNetworkOrCacheFetch ( !httpRequest.preventNoCacheCacheControlHeaderModification && !httpRequest.headersList.has('cache-control') ) { - httpRequest.headersList.append('cache-control', 'max-age=0') + httpRequest.headersList.set('cache-control', 'max-age=0') } // 17. If httpRequest’s cache mode is "no-store" or "reload", then: @@ -1369,20 +1369,20 @@ async function httpNetworkOrCacheFetch ( // 1. If httpRequest’s header list does not contain `Pragma`, then append // `Pragma`/`no-cache` to httpRequest’s header list. if (!httpRequest.headersList.has('pragma')) { - httpRequest.headersList.append('pragma', 'no-cache') + httpRequest.headersList.set('pragma', 'no-cache') } // 2. If httpRequest’s header list does not contain `Cache-Control`, // then append `Cache-Control`/`no-cache` to httpRequest’s header list. if (!httpRequest.headersList.has('cache-control')) { - httpRequest.headersList.append('cache-control', 'no-cache') + httpRequest.headersList.set('cache-control', 'no-cache') } } // 18. If httpRequest’s header list contains `Range`, then append // `Accept-Encoding`/`identity` to httpRequest’s header list. if (httpRequest.headersList.has('range')) { - httpRequest.headersList.append('accept-encoding', 'identity') + httpRequest.headersList.set('accept-encoding', 'identity') } // 19. Modify httpRequest’s header list per HTTP. Do not append a given @@ -1390,9 +1390,9 @@ async function httpNetworkOrCacheFetch ( // TODO: https://github.com/whatwg/fetch/issues/1285#issuecomment-896560129 if (!httpRequest.headersList.has('accept-encoding')) { if (/^https:/.test(requestCurrentURL(httpRequest).protocol)) { - httpRequest.headersList.append('accept-encoding', 'br, gzip, deflate') + httpRequest.headersList.set('accept-encoding', 'br, gzip, deflate') } else { - httpRequest.headersList.append('accept-encoding', 'gzip, deflate') + httpRequest.headersList.set('accept-encoding', 'gzip, deflate') } } @@ -1918,7 +1918,7 @@ async function httpNetworkFetch ( origin: url.origin, method: request.method, body: fetchParams.controller.dispatcher[kIsMockActive] ? request.body && request.body.source : body, - headers: request.headersList, + headers: [...request.headersList].flat(), maxRedirections: 0 }, { diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 93175d20eed..8e16d9299ec 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -464,6 +464,7 @@ class Request { // this’s headers. if (contentType && !this[kHeaders].has('content-type')) { this[kHeaders].append('content-type', contentType) + this[kState].headersList.append('content-type', contentType) } } diff --git a/lib/fetch/response.js b/lib/fetch/response.js index 3ecd28980eb..0e96c4497c8 100644 --- a/lib/fetch/response.js +++ b/lib/fetch/response.js @@ -81,7 +81,7 @@ class Response { const value = parsedURL.toString() // 7. Append `Location`/value to responseObject’s response’s header list. - responseObject[kState].headersList.push('location', value) + responseObject[kState].headersList.append('location', value) // 8. Return responseObject. return responseObject diff --git a/test/node-fetch/mock.js b/test/node-fetch/mock.js index d4881eabc35..a53f464a1a9 100644 --- a/test/node-fetch/mock.js +++ b/test/node-fetch/mock.js @@ -68,8 +68,8 @@ describe('node-fetch with MockAgent', () => { .intercept({ path: '/test', method: 'GET', - headers: { - 'User-Agent': /^undici$/ + headers: (h) => { + return h['user-agent'] === 'undici' } }) .reply(200, { success: true }) @@ -79,7 +79,6 @@ describe('node-fetch with MockAgent', () => { method: 'GET', headers: new Headers({ 'User-Agent': 'undici' }) }) - expect(res.status).to.equal(200) expect(await res.json()).to.deep.equal({ success: true }) }) From 2986c982d6681d70a2ea4a6e0173d31d93824844 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Thu, 7 Apr 2022 23:55:40 -0300 Subject: [PATCH 06/12] update: remove comment --- lib/fetch/request.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 8e16d9299ec..845344a7f9c 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -426,7 +426,6 @@ class Request { ]) } else { // 5. Otherwise, fill this’s headers with headers. - // TODO: check it before merge fillHeaders(this[kState].headersList, headers) } } From 0941bb5e01475d4a440de53fa1cf4cfcadb9b03b Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Thu, 7 Apr 2022 23:57:54 -0300 Subject: [PATCH 07/12] fix: lint --- lib/fetch/headers.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index a4294dafee8..513dcf8a6b2 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -78,7 +78,7 @@ function fill (headers, object) { } class HeadersList { - constructor(init) { + constructor (init) { if (init instanceof HeadersList) { this[kHeadersMap] = new Map(init[kHeadersMap]) this[kSorted] = init[kSorted] @@ -117,7 +117,7 @@ class HeadersList { return this[kHeadersMap].has(normalizedName) } - sortMap() { + sortMap () { this[kHeadersMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1)) this[kSorted] = true } @@ -127,7 +127,7 @@ class HeadersList { return this[kHeadersMap].set(key, val) } - *[Symbol.iterator]() { + * [Symbol.iterator] () { if (!this[kSorted]) { this.sortMap() } @@ -299,7 +299,7 @@ class Headers { // The value pairs to iterate over are the return value of running sort // and combine with this’s header list. - _getHeadersListIterable() { + _getHeadersListIterable () { if (!this[kHeadersList][kSorted]) { this[kHeadersList].sortMap() } From aceec4d3358505c3ae77cbd4c9ba250d55f09b3d Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Fri, 8 Apr 2022 11:19:16 -0300 Subject: [PATCH 08/12] update(headers): create a new headers map for sorting --- lib/fetch/headers.js | 28 ++++++++++++++++------------ lib/fetch/index.js | 12 ++++++------ test/fetch/headers.js | 3 ++- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 513dcf8a6b2..376c879b088 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -12,7 +12,8 @@ const { } = require('./constants') const kHeadersMap = Symbol('headers map') -const kSorted = Symbol('headers map sorted') +const kSorted = Symbol('kSorted') +const kHeadersSortedMap = Symbol('headers map sorted') function normalizeAndValidateHeaderName (name) { if (name === undefined) { @@ -82,9 +83,11 @@ class HeadersList { if (init instanceof HeadersList) { this[kHeadersMap] = new Map(init[kHeadersMap]) this[kSorted] = init[kSorted] + this[kHeadersSortedMap] = init[kHeadersSortedMap] } else { this[kHeadersMap] = new Map(init) this[kSorted] = false + this[kHeadersSortedMap] = new Map() } } @@ -118,12 +121,13 @@ class HeadersList { } sortMap () { - this[kHeadersMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1)) + this[kHeadersSortedMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1)) this[kSorted] = true } // bypass normalization set (key, val) { + this[kSorted] = false return this[kHeadersMap].set(key, val) } @@ -132,7 +136,7 @@ class HeadersList { this.sortMap() } - for (const [key, val] of this[kHeadersMap].entries()) { + for (const [key, val] of this[kHeadersSortedMap].entries()) { yield [key, val] } } @@ -304,26 +308,26 @@ class Headers { this[kHeadersList].sortMap() } - return new HeadersList(this[kHeadersList]) + return this[kHeadersList][kHeadersSortedMap] } * keys () { - const listFreezed = this._getHeadersListIterable() - for (const key of listFreezed[kHeadersMap].keys()) { + const sortedHeaders = this._getHeadersListIterable() + for (const key of sortedHeaders.keys()) { yield key } } * values () { - const listFreezed = this._getHeadersListIterable() - for (const val of listFreezed[kHeadersMap].values()) { + const sortedHeaders = this._getHeadersListIterable() + for (const val of sortedHeaders.values()) { yield val } } * entries () { - const listFreezed = this._getHeadersListIterable() - for (const [key, val] of listFreezed[kHeadersMap]) { + const sortedHeaders = this._getHeadersListIterable() + for (const [key, val] of sortedHeaders) { yield [key, val] } } @@ -345,8 +349,8 @@ class Headers { const callback = args[0] const thisArg = args[1] - const listFreezed = this._getHeadersListIterable() - listFreezed[kHeadersMap].forEach((value, index) => { + const sortedHeaders = this._getHeadersListIterable() + sortedHeaders.forEach((value, index) => { callback.apply(thisArg, [value, index, this]) }) } diff --git a/lib/fetch/index.js b/lib/fetch/index.js index 5ccc5c14e92..11caab91d01 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1361,7 +1361,7 @@ async function httpNetworkOrCacheFetch ( !httpRequest.preventNoCacheCacheControlHeaderModification && !httpRequest.headersList.has('cache-control') ) { - httpRequest.headersList.set('cache-control', 'max-age=0') + httpRequest.headersList.append('cache-control', 'max-age=0') } // 17. If httpRequest’s cache mode is "no-store" or "reload", then: @@ -1369,20 +1369,20 @@ async function httpNetworkOrCacheFetch ( // 1. If httpRequest’s header list does not contain `Pragma`, then append // `Pragma`/`no-cache` to httpRequest’s header list. if (!httpRequest.headersList.has('pragma')) { - httpRequest.headersList.set('pragma', 'no-cache') + httpRequest.headersList.append('pragma', 'no-cache') } // 2. If httpRequest’s header list does not contain `Cache-Control`, // then append `Cache-Control`/`no-cache` to httpRequest’s header list. if (!httpRequest.headersList.has('cache-control')) { - httpRequest.headersList.set('cache-control', 'no-cache') + httpRequest.headersList.append('cache-control', 'no-cache') } } // 18. If httpRequest’s header list contains `Range`, then append // `Accept-Encoding`/`identity` to httpRequest’s header list. if (httpRequest.headersList.has('range')) { - httpRequest.headersList.set('accept-encoding', 'identity') + httpRequest.headersList.append('accept-encoding', 'identity') } // 19. Modify httpRequest’s header list per HTTP. Do not append a given @@ -1390,9 +1390,9 @@ async function httpNetworkOrCacheFetch ( // TODO: https://github.com/whatwg/fetch/issues/1285#issuecomment-896560129 if (!httpRequest.headersList.has('accept-encoding')) { if (/^https:/.test(requestCurrentURL(httpRequest).protocol)) { - httpRequest.headersList.set('accept-encoding', 'br, gzip, deflate') + httpRequest.headersList.append('accept-encoding', 'br, gzip, deflate') } else { - httpRequest.headersList.set('accept-encoding', 'gzip, deflate') + httpRequest.headersList.append('accept-encoding', 'gzip, deflate') } } diff --git a/test/fetch/headers.js b/test/fetch/headers.js index 38f52c577bc..cf5bdbfe1aa 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -568,7 +568,8 @@ tap.test('node inspect', (t) => { headers.set('k2', 'v2') t.equal(util.inspect(headers), `HeadersList { [Symbol(headers map)]: Map(2) { 'k1' => 'v1', 'k2' => 'v2' }, - [Symbol(headers map sorted)]: false + [Symbol(kSorted)]: false, + [Symbol(headers map sorted)]: Map(0) {} }`) t.end() }) From 765fc314ca9d620da54955ec5548b138bedc85e3 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Fri, 8 Apr 2022 11:32:10 -0300 Subject: [PATCH 09/12] update: remove kSorted --- lib/fetch/headers.js | 14 +++++--------- test/fetch/headers.js | 3 +-- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 376c879b088..4666cf2e008 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -12,7 +12,6 @@ const { } = require('./constants') const kHeadersMap = Symbol('headers map') -const kSorted = Symbol('kSorted') const kHeadersSortedMap = Symbol('headers map sorted') function normalizeAndValidateHeaderName (name) { @@ -82,12 +81,10 @@ class HeadersList { constructor (init) { if (init instanceof HeadersList) { this[kHeadersMap] = new Map(init[kHeadersMap]) - this[kSorted] = init[kSorted] this[kHeadersSortedMap] = init[kHeadersSortedMap] } else { this[kHeadersMap] = new Map(init) - this[kSorted] = false - this[kHeadersSortedMap] = new Map() + this[kHeadersSortedMap] = undefined } } @@ -101,7 +98,7 @@ class HeadersList { this[kHeadersMap].set(normalizedName, `${exists}, ${normalizedValue}`) } else { this[kHeadersMap].set(normalizedName, `${normalizedValue}`) - this[kSorted] = false + this[kHeadersSortedMap] = undefined } } @@ -122,17 +119,16 @@ class HeadersList { sortMap () { this[kHeadersSortedMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1)) - this[kSorted] = true } // bypass normalization set (key, val) { - this[kSorted] = false + this[kHeadersSortedMap] = undefined return this[kHeadersMap].set(key, val) } * [Symbol.iterator] () { - if (!this[kSorted]) { + if (!this[kHeadersSortedMap]) { this.sortMap() } @@ -304,7 +300,7 @@ class Headers { // The value pairs to iterate over are the return value of running sort // and combine with this’s header list. _getHeadersListIterable () { - if (!this[kHeadersList][kSorted]) { + if (!this[kHeadersList][kHeadersSortedMap]) { this[kHeadersList].sortMap() } diff --git a/test/fetch/headers.js b/test/fetch/headers.js index cf5bdbfe1aa..f10b2604713 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -568,8 +568,7 @@ tap.test('node inspect', (t) => { headers.set('k2', 'v2') t.equal(util.inspect(headers), `HeadersList { [Symbol(headers map)]: Map(2) { 'k1' => 'v1', 'k2' => 'v2' }, - [Symbol(kSorted)]: false, - [Symbol(headers map sorted)]: Map(0) {} + [Symbol(headers map sorted)]: undefined }`) t.end() }) From 0fdf0a698b495d6148396a41ffd4682584203187 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Fri, 8 Apr 2022 19:58:02 -0300 Subject: [PATCH 10/12] update: replace .set to .append --- lib/fetch/index.js | 2 +- lib/fetch/response.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/fetch/index.js b/lib/fetch/index.js index 11caab91d01..9055d93b092 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1334,7 +1334,7 @@ async function httpNetworkOrCacheFetch ( // user agents should append `User-Agent`/default `User-Agent` value to // httpRequest’s header list. if (!httpRequest.headersList.has('user-agent')) { - httpRequest.headersList.set('user-agent', 'undici') + httpRequest.headersList.append('user-agent', 'undici') } // 15. If httpRequest’s cache mode is "default" and httpRequest’s header diff --git a/lib/fetch/response.js b/lib/fetch/response.js index 0e96c4497c8..f704ac87512 100644 --- a/lib/fetch/response.js +++ b/lib/fetch/response.js @@ -172,7 +172,7 @@ class Response { // not contain `Content-Type`, then append `Content-Type`/Content-Type // to this’s response’s header list. if (contentType && !this.headers.has('content-type')) { - this.headers.set('content-type', contentType) + this.headers.append('content-type', contentType) } } } From 6065226e78fb462e007f35acb581e4e8d8255616 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 9 Apr 2022 11:55:01 +0200 Subject: [PATCH 11/12] fixes --- lib/fetch/headers.js | 87 ++++++++++++++++++++++++------------------- lib/fetch/request.js | 5 +++ lib/fetch/response.js | 13 +++---- test/fetch/headers.js | 15 +------- 4 files changed, 59 insertions(+), 61 deletions(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 4666cf2e008..161555f94b8 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -84,11 +84,13 @@ class HeadersList { this[kHeadersSortedMap] = init[kHeadersSortedMap] } else { this[kHeadersMap] = new Map(init) - this[kHeadersSortedMap] = undefined + this[kHeadersSortedMap] = null } } append (name, value) { + this[kHeadersSortedMap] = null + const normalizedName = normalizeAndValidateHeaderName(name) const normalizedValue = normalizeAndValidateHeaderValue(name, value) @@ -98,11 +100,19 @@ class HeadersList { this[kHeadersMap].set(normalizedName, `${exists}, ${normalizedValue}`) } else { this[kHeadersMap].set(normalizedName, `${normalizedValue}`) - this[kHeadersSortedMap] = undefined } } + set (name, value) { + this[kHeadersSortedMap] = null + + const normalizedName = normalizeAndValidateHeaderName(name) + return this[kHeadersMap].set(normalizedName, value) + } + delete (name) { + this[kHeadersSortedMap] = null + const normalizedName = normalizeAndValidateHeaderName(name) return this[kHeadersMap].delete(normalizedName) } @@ -117,24 +127,20 @@ class HeadersList { return this[kHeadersMap].has(normalizedName) } - sortMap () { - this[kHeadersSortedMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1)) + keys() { + return this[kHeadersMap].keys() } - // bypass normalization - set (key, val) { - this[kHeadersSortedMap] = undefined - return this[kHeadersMap].set(key, val) + values () { + return this[kHeadersMap].values() } - * [Symbol.iterator] () { - if (!this[kHeadersSortedMap]) { - this.sortMap() - } + entries () { + return this[kHeadersMap].entries() + } - for (const [key, val] of this[kHeadersSortedMap].entries()) { - yield [key, val] - } + [Symbol.iterator] () { + return this[kHeadersMap][Symbol.iterator]() } } @@ -276,20 +282,18 @@ class Headers { ) } - const normalizedName = normalizeAndValidateHeaderName(String(args[0])) - if (this[kGuard] === 'immutable') { throw new TypeError('immutable') } else if ( this[kGuard] === 'request' && - forbiddenHeaderNames.includes(normalizedName) + forbiddenHeaderNames.includes(String(args[0]).toLocaleLowerCase()) ) { return } else if (this[kGuard] === 'request-no-cors') { // TODO } else if ( this[kGuard] === 'response' && - forbiddenResponseHeaderNames.includes(normalizedName) + forbiddenResponseHeaderNames.includes(String(args[0]).toLocaleLowerCase()) ) { return } @@ -297,35 +301,41 @@ class Headers { return this[kHeadersList].set(String(args[0]), String(args[1])) } - // The value pairs to iterate over are the return value of running sort - // and combine with this’s header list. - _getHeadersListIterable () { - if (!this[kHeadersList][kHeadersSortedMap]) { - this[kHeadersList].sortMap() + get [kHeadersSortedMap] () { + this[kHeadersList][kHeadersSortedMap] ??= new Map([...this[kHeadersList]].sort((a, b) => a[0] < b[0] ? -1 : 1)) + return this[kHeadersList][kHeadersSortedMap] + } + + keys () { + if (!(this instanceof Headers)) { + throw new TypeError('Illegal invocation') } - return this[kHeadersList][kHeadersSortedMap] + return this[kHeadersSortedMap].keys() } - * keys () { - const sortedHeaders = this._getHeadersListIterable() - for (const key of sortedHeaders.keys()) { - yield key + values () { + if (!(this instanceof Headers)) { + throw new TypeError('Illegal invocation') } + + return this[kHeadersSortedMap].values() } - * values () { - const sortedHeaders = this._getHeadersListIterable() - for (const val of sortedHeaders.values()) { - yield val + entries () { + if (!(this instanceof Headers)) { + throw new TypeError('Illegal invocation') } + + return this[kHeadersSortedMap].entries() } - * entries () { - const sortedHeaders = this._getHeadersListIterable() - for (const [key, val] of sortedHeaders) { - yield [key, val] + [Symbol.iterator] () { + if (!(this instanceof Headers)) { + throw new TypeError('Illegal invocation') } + + return this[kHeadersSortedMap] } forEach (...args) { @@ -345,8 +355,7 @@ class Headers { const callback = args[0] const thisArg = args[1] - const sortedHeaders = this._getHeadersListIterable() - sortedHeaders.forEach((value, index) => { + this[kHeadersSortedMap].forEach((value, index) => { callback.apply(thisArg, [value, index, this]) }) } diff --git a/lib/fetch/request.js b/lib/fetch/request.js index 845344a7f9c..0f10e678894 100644 --- a/lib/fetch/request.js +++ b/lib/fetch/request.js @@ -420,6 +420,11 @@ class Request { // 4. If headers is a Headers object, then for each header in its header // list, append header’s name/header’s value to this’s headers. if (headers instanceof Headers) { + // TODO (fix): Why doesn't this work? + // for (const [key, val] of headers[kHeadersList]) { + // this[kHeaders].append(key, val) + // } + this[kState].headersList = new HeadersList([ ...this[kState].headersList, ...headers[kHeadersList] diff --git a/lib/fetch/response.js b/lib/fetch/response.js index f704ac87512..d1ce6ce95bd 100644 --- a/lib/fetch/response.js +++ b/lib/fetch/response.js @@ -394,16 +394,13 @@ function makeFilteredHeadersList (headersList, filter) { // Override methods used by Headers class. if (prop === 'get' || prop === 'has') { return (name) => filter(name) ? target[prop](name) : undefined - } else if (prop === 'slice') { - return (...args) => { - assert(args.length === 0) - const arr = [] - for (let index = 0; index < target.length; index += 2) { - if (filter(target[index])) { - arr.push(target[index], target[index + 1]) + } else if (prop === Symbol.iterator) { + return function * () { + for (const entry of target) { + if (filter(entry[0])) { + yield entry } } - return arr } } else { return target[prop] diff --git a/test/fetch/headers.js b/test/fetch/headers.js index f10b2604713..4c4e2171915 100644 --- a/test/fetch/headers.js +++ b/test/fetch/headers.js @@ -1,6 +1,5 @@ 'use strict' -const util = require('util') const tap = require('tap') const { Headers, @@ -8,7 +7,6 @@ const { normalizeAndValidateHeaderValue, fill } = require('../../lib/fetch/headers') -const { kHeadersList } = require('../../lib/core/symbols') const { kGuard } = require('../../lib/fetch/symbols') const { forbiddenHeaderNames, @@ -454,7 +452,7 @@ tap.test('Headers as Iterable', t => { ['f', '6'] ])] - t.same([...headers[kHeadersList]], expected) + t.same([...headers], expected) }) }) @@ -562,17 +560,6 @@ tap.test('various init paths of Headers', (t) => { t.end() }) -tap.test('node inspect', (t) => { - const headers = new Headers() - headers.set('k1', 'v1') - headers.set('k2', 'v2') - t.equal(util.inspect(headers), `HeadersList { - [Symbol(headers map)]: Map(2) { 'k1' => 'v1', 'k2' => 'v2' }, - [Symbol(headers map sorted)]: undefined -}`) - t.end() -}) - tap.test('immutable guard', (t) => { const headers = new Headers() headers.set('key', 'val') From f604e28667a798102e33c1147b9402801e1b7653 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Sat, 9 Apr 2022 10:03:44 -0300 Subject: [PATCH 12/12] fix: lint --- lib/fetch/headers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fetch/headers.js b/lib/fetch/headers.js index 161555f94b8..e0295be2250 100644 --- a/lib/fetch/headers.js +++ b/lib/fetch/headers.js @@ -127,7 +127,7 @@ class HeadersList { return this[kHeadersMap].has(normalizedName) } - keys() { + keys () { return this[kHeadersMap].keys() }