fetch: replace HeadersList Array inheritance to Map (nodejs#1309)

* update: uses Map over Array in HeadersList * fix: create empty HeaderList * update(fetch/headers): sort keys before returning * update: remove binarySearch method * perf: use headersList.set to bypass validation * update: remove comment * fix: lint * update(headers): create a new headers map for sorting * update: remove kSorted * update: replace .set to .append * fixes * fix: lint Co-authored-by: Robert Nagy <[email protected]>
metcoder95 · Dec 26, 2022 · 6de3b6e · 6de3b6e
1 parent e480f52
commit 6de3b6e
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 155 deletions.
diff --git 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 kHeadersSortedMap = Symbol('headers map sorted')
 
 function normalizeAndValidateHeaderName (name) {
   if (name === undefined) {
@@ -91,64 +77,74 @@ 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[kHeadersSortedMap] = init[kHeadersSortedMap]
+    } else {
+      this[kHeadersMap] = new Map(init)
+      this[kHeadersSortedMap] = null
+    }
+  }
+
   append (name, value) {
+    this[kHeadersSortedMap] = null
+
     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}`)
     }
   }
 
-  delete (name) {
+  set (name, value) {
+    this[kHeadersSortedMap] = null
+
     const normalizedName = normalizeAndValidateHeaderName(name)
+    return this[kHeadersMap].set(normalizedName, value)
+  }
 
-    const index = binarySearch(this, normalizedName)
+  delete (name) {
+    this[kHeadersSortedMap] = null
 
-    if (this[index] === normalizedName) {
-      this.splice(index, 2)
-    }
+    const normalizedName = normalizeAndValidateHeaderName(name)
+    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)
+  keys () {
+    return this[kHeadersMap].keys()
+  }
 
-    return this[index] === normalizedName
+  values () {
+    return this[kHeadersMap].values()
   }
 
-  set (name, value) {
-    const normalizedName = normalizeAndValidateHeaderName(name)
-    const normalizedValue = normalizeAndValidateHeaderValue(name, value)
+  entries () {
+    return this[kHeadersMap].entries()
+  }
 
-    const index = binarySearch(this, normalizedName)
-    if (this[index] === normalizedName) {
-      this[index + 1] = normalizedValue
-    } else {
-      this.splice(index, 0, normalizedName, normalizedValue)
-    }
+  [Symbol.iterator] () {
+    return this[kHeadersMap][Symbol.iterator]()
   }
 }
 
+// https://fetch.spec.whatwg.org/#headers-class
 class Headers {
   constructor (...args) {
     if (
@@ -161,7 +157,6 @@ class Headers {
       )
     }
     const init = args.length >= 1 ? args[0] ?? {} : {}
-
     this[kHeadersList] = new HeadersList()
 
     // The new Headers(init) constructor steps are:
@@ -287,46 +282,60 @@ 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
     }
 
     return this[kHeadersList].set(String(args[0]), String(args[1]))
   }
 
-  * keys () {
-    const clone = this[kHeadersList].slice()
-    for (let index = 0; index < clone.length; index += 2) {
-      yield clone[index]
+  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[kHeadersSortedMap].keys()
   }
 
-  * values () {
-    const clone = this[kHeadersList].slice()
-    for (let index = 1; index < clone.length; index += 2) {
-      yield clone[index]
+  values () {
+    if (!(this instanceof Headers)) {
+      throw new TypeError('Illegal invocation')
     }
+
+    return this[kHeadersSortedMap].values()
   }
 
-  * entries () {
-    const clone = this[kHeadersList].slice()
-    for (let index = 0; index < clone.length; index += 2) {
-      yield [clone[index], clone[index + 1]]
+  entries () {
+    if (!(this instanceof Headers)) {
+      throw new TypeError('Illegal invocation')
     }
+
+    return this[kHeadersSortedMap].entries()
+  }
+
+  [Symbol.iterator] () {
+    if (!(this instanceof Headers)) {
+      throw new TypeError('Illegal invocation')
+    }
+
+    return this[kHeadersSortedMap]
   }
 
   forEach (...args) {
@@ -346,15 +355,9 @@ 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[kHeadersSortedMap].forEach((value, index) => {
+      callback.apply(thisArg, [value, index, this])
+    })
   }
 
   [Symbol.for('nodejs.util.inspect.custom')] () {
@@ -384,7 +387,6 @@ module.exports = {
   fill,
   Headers,
   HeadersList,
-  binarySearch,
   normalizeAndValidateHeaderName,
   normalizeAndValidateHeaderValue
 }
diff --git 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]
       })
@@ -1919,7 +1919,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
@@ -420,7 +420,15 @@ 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])
+        // 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]
+        ])
       } else {
         // 5. Otherwise, fill this’s headers with headers.
         fillHeaders(this[kState].headersList, headers)
@@ -460,6 +468,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)
       }
     }
 
@@ -793,7 +802,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
@@ -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
@@ -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)
       }
     }
   }
@@ -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] : []
   }
@@ -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]