Skip to content

Commit

Permalink
use private properties in Headers (#3269)
Browse files Browse the repository at this point in the history
  • Loading branch information
KhafraDev authored May 16, 2024
1 parent 106bf1c commit 4da45f9
Show file tree
Hide file tree
Showing 13 changed files with 119 additions and 83 deletions.
8 changes: 2 additions & 6 deletions benchmarks/fetch/headers-length32.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { bench, run } from 'mitata'
import { Headers } from '../../lib/web/fetch/headers.js'
import { Headers, getHeadersList } from '../../lib/web/fetch/headers.js'

const headers = new Headers(
[
Expand Down Expand Up @@ -38,11 +38,7 @@ const headers = new Headers(
].map((v) => [v, ''])
)

const kHeadersList = Reflect.ownKeys(headers).find(
(c) => String(c) === 'Symbol(headers list)'
)

const headersList = headers[kHeadersList]
const headersList = getHeadersList(headers)

const kHeadersSortedMap = Reflect.ownKeys(headersList).find(
(c) => String(c) === 'Symbol(headers map sorted)'
Expand Down
10 changes: 3 additions & 7 deletions benchmarks/fetch/headers.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { bench, group, run } from 'mitata'
import { Headers } from '../../lib/web/fetch/headers.js'
import { Headers, getHeadersList } from '../../lib/web/fetch/headers.js'

const characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'
const charactersLength = characters.length
Expand Down Expand Up @@ -27,13 +27,9 @@ for (const [name, length] of Object.entries(settings)) {

const headersSorted = new Headers(headers)

const kHeadersList = Reflect.ownKeys(headers).find(
(c) => String(c) === 'Symbol(headers list)'
)

const headersList = headers[kHeadersList]
const headersList = getHeadersList(headers)

const headersListSorted = headersSorted[kHeadersList]
const headersListSorted = getHeadersList(headersSorted)

const kHeadersSortedMap = Reflect.ownKeys(headersList).find(
(c) => String(c) === 'Symbol(headers map sorted)'
Expand Down
1 change: 0 additions & 1 deletion lib/core/symbols.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ module.exports = {
kQueue: Symbol('queue'),
kConnect: Symbol('connect'),
kConnecting: Symbol('connecting'),
kHeadersList: Symbol('headers list'),
kKeepAliveDefaultTimeout: Symbol('default keep alive timeout'),
kKeepAliveMaxTimeout: Symbol('max keep alive timeout'),
kKeepAliveTimeoutThreshold: Symbol('keep alive timeout threshold'),
Expand Down
8 changes: 5 additions & 3 deletions lib/web/cookies/util.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict'

const assert = require('node:assert')
const { kHeadersList } = require('../../core/symbols')
const { getHeadersList: internalGetHeadersList } = require('../fetch/headers')

/**
* @param {string} value
Expand Down Expand Up @@ -278,8 +278,10 @@ function stringify (cookie) {
let kHeadersListNode

function getHeadersList (headers) {
if (headers[kHeadersList]) {
return headers[kHeadersList]
try {
return internalGetHeadersList(headers)
} catch {
// fall-through
}

if (!kHeadersListNode) {
Expand Down
90 changes: 59 additions & 31 deletions lib/web/fetch/headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

'use strict'

const { kHeadersList, kConstruct } = require('../../core/symbols')
const { kGuard } = require('./symbols')
const { kConstruct } = require('../../core/symbols')
const { kEnumerableProperty } = require('../../core/util')
const {
iteratorMixin,
Expand Down Expand Up @@ -103,19 +102,18 @@ function appendHeader (headers, name, value) {
// 3. If headers’s guard is "immutable", then throw a TypeError.
// 4. Otherwise, if headers’s guard is "request" and name is a
// forbidden header name, return.
// 5. Otherwise, if headers’s guard is "request-no-cors":
// TODO
// Note: undici does not implement forbidden header names
if (headers[kGuard] === 'immutable') {
if (getHeadersGuard(headers) === 'immutable') {
throw new TypeError('immutable')
} else if (headers[kGuard] === 'request-no-cors') {
// 5. Otherwise, if headers’s guard is "request-no-cors":
// TODO
}

// 6. Otherwise, if headers’s guard is "response" and name is a
// forbidden response-header name, return.

// 7. Append (name, value) to headers’s header list.
return headers[kHeadersList].append(name, value, false)
return getHeadersList(headers).append(name, value, false)

// 8. If headers’s guard is "request-no-cors", then remove
// privileged no-CORS request headers from headers
Expand Down Expand Up @@ -357,16 +355,20 @@ class HeadersList {

// https://fetch.spec.whatwg.org/#headers-class
class Headers {
#guard
#headersList

constructor (init = undefined) {
if (init === kConstruct) {
return
}
this[kHeadersList] = new HeadersList()

this.#headersList = new HeadersList()

// The new Headers(init) constructor steps are:

// 1. Set this’s guard to "none".
this[kGuard] = 'none'
this.#guard = 'none'

// 2. If init is given, then fill this with init.
if (init !== undefined) {
Expand Down Expand Up @@ -416,22 +418,20 @@ class Headers {
// 5. Otherwise, if this’s guard is "response" and name is
// a forbidden response-header name, return.
// Note: undici does not implement forbidden header names
if (this[kGuard] === 'immutable') {
if (this.#guard === 'immutable') {
throw new TypeError('immutable')
} else if (this[kGuard] === 'request-no-cors') {
// TODO
}

// 6. If this’s header list does not contain name, then
// return.
if (!this[kHeadersList].contains(name, false)) {
if (!this.#headersList.contains(name, false)) {
return
}

// 7. Delete name from this’s header list.
// 8. If this’s guard is "request-no-cors", then remove
// privileged no-CORS request headers from this.
this[kHeadersList].delete(name, false)
this.#headersList.delete(name, false)
}

// https://fetch.spec.whatwg.org/#dom-headers-get
Expand All @@ -454,7 +454,7 @@ class Headers {

// 2. Return the result of getting name from this’s header
// list.
return this[kHeadersList].get(name, false)
return this.#headersList.get(name, false)
}

// https://fetch.spec.whatwg.org/#dom-headers-has
Expand All @@ -477,7 +477,7 @@ class Headers {

// 2. Return true if this’s header list contains name;
// otherwise false.
return this[kHeadersList].contains(name, false)
return this.#headersList.contains(name, false)
}

// https://fetch.spec.whatwg.org/#dom-headers-set
Expand Down Expand Up @@ -518,16 +518,14 @@ class Headers {
// 6. Otherwise, if this’s guard is "response" and name is a
// forbidden response-header name, return.
// Note: undici does not implement forbidden header names
if (this[kGuard] === 'immutable') {
if (this.#guard === 'immutable') {
throw new TypeError('immutable')
} else if (this[kGuard] === 'request-no-cors') {
// TODO
}

// 7. Set (name, value) in this’s header list.
// 8. If this’s guard is "request-no-cors", then remove
// privileged no-CORS request headers from this
this[kHeadersList].set(name, value, false)
this.#headersList.set(name, value, false)
}

// https://fetch.spec.whatwg.org/#dom-headers-getsetcookie
Expand All @@ -538,7 +536,7 @@ class Headers {
// 2. Return the values of all headers in this’s header list whose name is
// a byte-case-insensitive match for `Set-Cookie`, in order.

const list = this[kHeadersList].cookies
const list = this.#headersList.cookies

if (list) {
return [...list]
Expand All @@ -549,8 +547,8 @@ class Headers {

// https://fetch.spec.whatwg.org/#concept-header-list-sort-and-combine
get [kHeadersSortedMap] () {
if (this[kHeadersList][kHeadersSortedMap]) {
return this[kHeadersList][kHeadersSortedMap]
if (this.#headersList[kHeadersSortedMap]) {
return this.#headersList[kHeadersSortedMap]
}

// 1. Let headers be an empty list of headers with the key being the name
Expand All @@ -559,14 +557,14 @@ class Headers {

// 2. Let names be the result of convert header names to a sorted-lowercase
// set with all the names of the headers in list.
const names = this[kHeadersList].toSortedArray()
const names = this.#headersList.toSortedArray()

const cookies = this[kHeadersList].cookies
const cookies = this.#headersList.cookies

// fast-path
if (cookies === null || cookies.length === 1) {
// Note: The non-null assertion of value has already been done by `HeadersList#toSortedArray`
return (this[kHeadersList][kHeadersSortedMap] = names)
return (this.#headersList[kHeadersSortedMap] = names)
}

// 3. For each name of names:
Expand Down Expand Up @@ -596,16 +594,38 @@ class Headers {
}

// 4. Return headers.
return (this[kHeadersList][kHeadersSortedMap] = headers)
return (this.#headersList[kHeadersSortedMap] = headers)
}

[util.inspect.custom] (depth, options) {
options.depth ??= depth

return `Headers ${util.formatWithOptions(options, this[kHeadersList].entries)}`
return `Headers ${util.formatWithOptions(options, this.#headersList.entries)}`
}

static getHeadersGuard (o) {
return o.#guard
}

static setHeadersGuard (o, guard) {
o.#guard = guard
}

static getHeadersList (o) {
return o.#headersList
}

static setHeadersList (o, list) {
o.#headersList = list
}
}

const { getHeadersGuard, setHeadersGuard, getHeadersList, setHeadersList } = Headers
Reflect.deleteProperty(Headers, 'getHeadersGuard')
Reflect.deleteProperty(Headers, 'setHeadersGuard')
Reflect.deleteProperty(Headers, 'getHeadersList')
Reflect.deleteProperty(Headers, 'setHeadersList')

Object.defineProperty(Headers.prototype, util.inspect.custom, {
enumerable: false
})
Expand All @@ -631,8 +651,12 @@ webidl.converters.HeadersInit = function (V, prefix, argument) {

// A work-around to ensure we send the properly-cased Headers when V is a Headers object.
// Read https://github.com/nodejs/undici/pull/3159#issuecomment-2075537226 before touching, please.
if (!util.types.isProxy(V) && kHeadersList in V && iterator === Headers.prototype.entries) { // Headers object
return V[kHeadersList].entriesList
if (!util.types.isProxy(V) && iterator === Headers.prototype.entries) { // Headers object
try {
return getHeadersList(V).entriesList
} catch {
// fall-through
}
}

if (typeof iterator === 'function') {
Expand All @@ -654,5 +678,9 @@ module.exports = {
// for test.
compareHeaderName,
Headers,
HeadersList
HeadersList,
getHeadersGuard,
setHeadersGuard,
setHeadersList,
getHeadersList
}
22 changes: 11 additions & 11 deletions lib/web/fetch/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
'use strict'

const { extractBody, mixinBody, cloneBody } = require('./body')
const { Headers, fill: fillHeaders, HeadersList } = require('./headers')
const { Headers, fill: fillHeaders, HeadersList, setHeadersGuard, getHeadersGuard, setHeadersList, getHeadersList } = require('./headers')
const { FinalizationRegistry } = require('./dispatcher-weakref')()
const util = require('../../core/util')
const nodeUtil = require('node:util')
Expand All @@ -25,10 +25,10 @@ const {
requestDuplex
} = require('./constants')
const { kEnumerableProperty } = util
const { kHeaders, kSignal, kState, kGuard, kDispatcher } = require('./symbols')
const { kHeaders, kSignal, kState, kDispatcher } = require('./symbols')
const { webidl } = require('./webidl')
const { URLSerializer } = require('./data-url')
const { kHeadersList, kConstruct } = require('../../core/symbols')
const { kConstruct } = require('../../core/symbols')
const assert = require('node:assert')
const { getMaxListeners, setMaxListeners, getEventListeners, defaultMaxListeners } = require('node:events')

Expand Down Expand Up @@ -445,8 +445,8 @@ class Request {
// Realm, whose header list is request’s header list and guard is
// "request".
this[kHeaders] = new Headers(kConstruct)
this[kHeaders][kHeadersList] = request.headersList
this[kHeaders][kGuard] = 'request'
setHeadersList(this[kHeaders], request.headersList)
setHeadersGuard(this[kHeaders], 'request')

// 31. If this’s request’s mode is "no-cors", then:
if (mode === 'no-cors') {
Expand All @@ -459,13 +459,13 @@ class Request {
}

// 2. Set this’s headers’s guard to "request-no-cors".
this[kHeaders][kGuard] = 'request-no-cors'
setHeadersGuard(this[kHeaders], 'request-no-cors')
}

// 32. If init is not empty, then:
if (initHasKey) {
/** @type {HeadersList} */
const headersList = this[kHeaders][kHeadersList]
const headersList = getHeadersList(this[kHeaders])
// 1. Let headers be a copy of this’s headers and its associated header
// list.
// 2. If init["headers"] exists, then set headers to init["headers"].
Expand Down Expand Up @@ -519,7 +519,7 @@ class Request {
// 3, If Content-Type is non-null and this’s headers’s header list does
// not contain `Content-Type`, then append `Content-Type`/Content-Type to
// this’s headers.
if (contentType && !this[kHeaders][kHeadersList].contains('content-type', true)) {
if (contentType && !getHeadersList(this[kHeaders]).contains('content-type', true)) {
this[kHeaders].append('content-type', contentType)
}
}
Expand Down Expand Up @@ -785,7 +785,7 @@ class Request {
}

// 4. Return clonedRequestObject.
return fromInnerRequest(clonedRequest, ac.signal, this[kHeaders][kGuard])
return fromInnerRequest(clonedRequest, ac.signal, getHeadersGuard(this[kHeaders]))
}

[nodeUtil.inspect.custom] (depth, options) {
Expand Down Expand Up @@ -894,8 +894,8 @@ function fromInnerRequest (innerRequest, signal, guard) {
request[kState] = innerRequest
request[kSignal] = signal
request[kHeaders] = new Headers(kConstruct)
request[kHeaders][kHeadersList] = innerRequest.headersList
request[kHeaders][kGuard] = guard
setHeadersList(request[kHeaders], innerRequest.headersList)
setHeadersGuard(request[kHeaders], guard)
return request
}

Expand Down
Loading

0 comments on commit 4da45f9

Please sign in to comment.