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

fetch: replace HeadersList Array inheritance to Map #1309

Merged
merged 12 commits into from
Apr 9, 2022
129 changes: 63 additions & 66 deletions lib/fetch/headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,9 @@ 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('kSorted')
const kHeadersSortedMap = Symbol('headers map sorted')

function normalizeAndValidateHeaderName (name) {
if (name === undefined) {
Expand Down Expand Up @@ -91,64 +78,71 @@ function fill (headers, object) {
}
}

// TODO: Composition over inheritence? Or helper methods?
class HeadersList extends Array {
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()
RafaelGSS marked this conversation as resolved.
Show resolved Hide resolved
}
}

append (name, value) {
const normalizedName = normalizeAndValidateHeaderName(name)
const normalizedValue = normalizeAndValidateHeaderValue(name, value)

const index = binarySearch(this, normalizedName)
const exists = this[kHeadersMap].get(normalizedName)

if (this[index] === normalizedName) {
this[index + 1] += `, ${normalizedValue}`
if (exists) {
this[kHeadersMap].set(normalizedName, `${exists}, ${normalizedValue}`)
} else {
this.splice(index, 0, normalizedName, normalizedValue)
this[kHeadersMap].set(normalizedName, `${normalizedValue}`)
this[kSorted] = false
}
}

delete (name) {
const normalizedName = normalizeAndValidateHeaderName(name)

const index = binarySearch(this, normalizedName)

if (this[index] === normalizedName) {
this.splice(index, 2)
}
return this[kHeadersMap].delete(normalizedName)
}

get (name) {
const normalizedName = normalizeAndValidateHeaderName(name)

const index = binarySearch(this, normalizedName)

if (this[index] === normalizedName) {
return this[index + 1]
}

return null
return this[kHeadersMap].get(normalizedName) ?? null
}

has (name) {
const normalizedName = normalizeAndValidateHeaderName(name)
return this[kHeadersMap].has(normalizedName)
}

const index = binarySearch(this, normalizedName)
sortMap () {
this[kHeadersSortedMap] = new Map([...this[kHeadersMap]].sort((a, b) => a[0] < b[0] ? -1 : 1))
this[kSorted] = true
}

return this[index] === normalizedName
// bypass normalization
set (key, val) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
this[kSorted] = false
return this[kHeadersMap].set(key, val)
ronag marked this conversation as resolved.
Show resolved Hide resolved
}

set (name, value) {
const normalizedName = normalizeAndValidateHeaderName(name)
const normalizedValue = normalizeAndValidateHeaderValue(name, value)
* [Symbol.iterator] () {
if (!this[kSorted]) {
this.sortMap()
Copy link
Member

Choose a reason for hiding this comment

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

I think you can inline sortMap?

Copy link
Member Author

@RafaelGSS RafaelGSS Apr 8, 2022

Choose a reason for hiding this comment

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

You mean?

if (!this[kHeadersSortedMap]) this.sortMap()

}

const index = binarySearch(this, normalizedName)
if (this[index] === normalizedName) {
this[index + 1] = normalizedValue
} else {
this.splice(index, 0, normalizedName, normalizedValue)
for (const [key, val] of this[kHeadersSortedMap].entries()) {
yield [key, val]
}
}
}

// https://fetch.spec.whatwg.org/#headers-class
class Headers {
constructor (...args) {
if (
Expand All @@ -161,7 +155,6 @@ class Headers {
)
}
const init = args.length >= 1 ? args[0] ?? {} : {}

this[kHeadersList] = new HeadersList()

// The new Headers(init) constructor steps are:
Expand Down Expand Up @@ -308,24 +301,34 @@ 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][kSorted]) {
this[kHeadersList].sortMap()
}

return this[kHeadersList][kHeadersSortedMap]
}

* keys () {
const clone = this[kHeadersList].slice()
for (let index = 0; index < clone.length; index += 2) {
yield clone[index]
const sortedHeaders = this._getHeadersListIterable()
for (const key of sortedHeaders.keys()) {
yield key
}
}

* values () {
const clone = this[kHeadersList].slice()
for (let index = 1; index < clone.length; index += 2) {
yield clone[index]
const sortedHeaders = this._getHeadersListIterable()
for (const val of sortedHeaders.values()) {
yield val
}
}

* entries () {
const clone = this[kHeadersList].slice()
for (let index = 0; index < clone.length; index += 2) {
yield [clone[index], clone[index + 1]]
const sortedHeaders = this._getHeadersListIterable()
for (const [key, val] of sortedHeaders) {
yield [key, val]
}
}

Expand All @@ -346,15 +349,10 @@ 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
)
}
const sortedHeaders = this._getHeadersListIterable()
sortedHeaders.forEach((value, index) => {
callback.apply(thisArg, [value, index, this])
})
}

[Symbol.for('nodejs.util.inspect.custom')] () {
Expand Down Expand Up @@ -384,7 +382,6 @@ module.exports = {
fill,
Headers,
HeadersList,
binarySearch,
normalizeAndValidateHeaderName,
normalizeAndValidateHeaderValue
}
8 changes: 4 additions & 4 deletions lib/fetch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']
]
})

Expand Down Expand Up @@ -871,7 +871,7 @@ async function schemeFetch (fetchParams) {
return makeResponse({
statusText: 'OK',
headersList: [
'content-type', contentType
['content-type', contentType]
],
body: extractBody(dataURLStruct.body)[0]
})
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
},
{
Expand Down
8 changes: 6 additions & 2 deletions lib/fetch/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,10 @@ 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.
fillHeaders(this[kState].headersList, headers)
Expand Down Expand Up @@ -460,6 +463,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)
}
}

Expand Down Expand Up @@ -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))] : []
}
Expand Down
4 changes: 2 additions & 2 deletions lib/fetch/response.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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] : []
}
Expand Down
Loading