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.

Sign up for GitHub

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

Jump to bottom

refactor: update Reply module #4072

Merged
merged 6 commits into from Jul 1, 2022
Merged

refactor: update Reply module #4072

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c544629
refactor: update Reply module
denshakhov Jun 22, 2022
d1fe724
refactor: partial revert
denshakhov Jun 23, 2022
4573e1c
refactor: revert for in loop
denshakhov Jun 23, 2022
6602564
refactor: if condition
denshakhov Jun 27, 2022
7b515d1
refactor: partial revert
denshakhov Jun 27, 2022
c905725
Merge branch 'fastify:main' into refactor-reply
denshakhov Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
109 changes: 49 additions & 60 deletions lib/reply.js
View file
Open in desktop
Expand Up @@ -135,15 +135,15 @@ Reply.prototype.send = function (payload) {
}

if (Buffer.isBuffer(payload)) {
denshakhov marked this conversation as resolved.
Show resolved Hide resolved
if (hasContentType === false) {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.OCTET
if (!hasContentType) {
jsumners marked this conversation as resolved.
Show resolved Hide resolved
this.type(CONTENT_TYPE.OCTET)
}
onSendHook(this, payload)
return this
}

if (hasContentType === false && typeof payload === 'string') {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.PLAIN
if (!hasContentType && typeof payload === 'string') {
jsumners marked this conversation as resolved.
Show resolved Hide resolved
this.type(CONTENT_TYPE.PLAIN)
jsumners marked this conversation as resolved.
Show resolved Hide resolved
onSendHook(this, payload)
return this
}
Expand All @@ -157,19 +157,20 @@ Reply.prototype.send = function (payload) {
payload = this[kReplySerializer](payload)
}

// The indexOf below also matches custom json mimetypes such as 'application/hal+json' or 'application/ld+json'
} else if (hasContentType === false || contentType.indexOf('json') > -1) {
if (hasContentType === false) {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.JSON
// The 'includes' below also matches custom json mimetypes such as 'application/hal+json' or 'application/ld+json'
} else if (!hasContentType || contentType.includes('json')) {
jsumners marked this conversation as resolved.
Show resolved Hide resolved
if (!hasContentType) {
jsumners marked this conversation as resolved.
Show resolved Hide resolved
this.type(CONTENT_TYPE.JSON)
} else {
// If user doesn't set charset, we will set charset to utf-8
if (contentType.indexOf('charset') === -1) {
if (!contentType.includes('charset')) {
jsumners marked this conversation as resolved.
Show resolved Hide resolved
const customContentType = contentType.trim()

Copy link
Member

Choose a reason for hiding this comment

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

What is happening here? Is this a line ending change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, just a new line to separate var definition and if condition (I added it when I was refactoring this reverted block of code). Delete?

if (customContentType.endsWith(';')) {
// custom content-type is ended with ';'
this[kReplyHeaders]['content-type'] = `${customContentType} charset=utf-8`
this.type(`${customContentType} charset=utf-8`)
} else {
this[kReplyHeaders]['content-type'] = `${customContentType}; charset=utf-8`
this.type(`${customContentType}; charset=utf-8`)
denshakhov marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand All @@ -186,12 +187,8 @@ Reply.prototype.send = function (payload) {

Reply.prototype.getHeader = function (key) {
key = key.toLowerCase()
const res = this.raw
let value = this[kReplyHeaders][key]
if (value === undefined && res.hasHeader(key)) {
value = res.getHeader(key)
}
return value

return this[kReplyHeaders][key] ?? this.raw.getHeader(key)
jsumners marked this conversation as resolved.
Show resolved Hide resolved
}

Reply.prototype.getHeaders = function () {
Expand All @@ -203,10 +200,8 @@ Reply.prototype.getHeaders = function () {

Reply.prototype.hasHeader = function (key) {
key = key.toLowerCase()
if (this[kReplyHeaders][key] !== undefined) {
return true
}
return this.raw.hasHeader(key)

return this[kReplyHeaders][key] !== undefined || this.raw.hasHeader(key)
}

Reply.prototype.removeHeader = function (key) {
Expand All @@ -216,35 +211,32 @@ Reply.prototype.removeHeader = function (key) {
return this
}

Reply.prototype.header = function (key, value) {
const _key = key.toLowerCase()

// default the value to ''
value = value === undefined ? '' : value
Reply.prototype.header = function (key, value = '') {
key = key.toLowerCase()

if (this[kReplyHeaders][_key] && _key === 'set-cookie') {
if (this[kReplyHeaders][key] && key === 'set-cookie') {
// https://tools.ietf.org/html/rfc7230#section-3.2.2
if (typeof this[kReplyHeaders][_key] === 'string') {
this[kReplyHeaders][_key] = [this[kReplyHeaders][_key]]
if (typeof this[kReplyHeaders][key] === 'string') {
this[kReplyHeaders][key] = [this[kReplyHeaders][key]]
}

if (Array.isArray(value)) {
Array.prototype.push.apply(this[kReplyHeaders][_key], value)
this[kReplyHeaders][key].push(...value)
} else {
this[kReplyHeaders][_key].push(value)
this[kReplyHeaders][key].push(value)
}
} else {
this[kReplyHeaders][_key] = value
this[kReplyHeaders][key] = value
}

return this
}

Reply.prototype.headers = function (headers) {
const keys = Object.keys(headers)
/* eslint-disable no-var */
for (var i = 0; i !== keys.length; ++i) {
const key = keys[i]
this.header(key, headers[key])
jsumners marked this conversation as resolved.
Show resolved Hide resolved
for (const headerName in headers) {
Copy link
Member

Choose a reason for hiding this comment

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

There is a significant difference between Object.keys and for...in:

for...in https://262.ecma-international.org/13.0/#sec-createforiniterator:

It is used to create a For-In Iterator object which iterates over the own and inherited enumerable string properties of object in a specific order.

Object.keys https://262.ecma-international.org/13.0/#sec-object.keys:

    1. Let nameList be ? EnumerableOwnPropertyNames(obj, key).

In short, the Object.keys implementation does not enumerate properties from higher up in the prototype chain. It only iterates the actual headers object properties.

this.header(headerName, headers[headerName])
}

return this
}

Expand All @@ -267,25 +259,30 @@ const INVALID_TRAILERS = new Set([

Reply.prototype.trailer = function (key, fn) {
key = key.toLowerCase()

if (INVALID_TRAILERS.has(key)) {
throw new FST_ERR_BAD_TRAILER_NAME(key)
}

if (typeof fn !== 'function') {
throw new FST_ERR_BAD_TRAILER_VALUE(key, typeof fn)
}
if (this[kReplyTrailers] === null) this[kReplyTrailers] = {}

this[kReplyTrailers] = this[kReplyTrailers] ?? {}
this[kReplyTrailers][key] = fn

return this
}

Reply.prototype.hasTrailer = function (key) {
if (this[kReplyTrailers] === null) return false
return this[kReplyTrailers][key.toLowerCase()] !== undefined
return !!this[kReplyTrailers]?.[key.toLowerCase()]
jsumners marked this conversation as resolved.
Show resolved Hide resolved
}

Reply.prototype.removeTrailer = function (key) {
if (this[kReplyTrailers] === null) return this
this[kReplyTrailers][key.toLowerCase()] = undefined
if (this[kReplyTrailers] !== null) {
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 I add it as a early exit approach.
The readability didn't change a lot when here is few line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

this[kReplyTrailers][key.toLowerCase()] = undefined
}

return this
}

Expand Down Expand Up @@ -320,8 +317,7 @@ Reply.prototype.serializer = function (fn) {
}

Reply.prototype.type = function (type) {
this[kReplyHeaders]['content-type'] = type
return this
return this.header('content-type', type)
denshakhov marked this conversation as resolved.
Show resolved Hide resolved
}

Reply.prototype.redirect = function (code, url) {
Expand All @@ -330,8 +326,7 @@ Reply.prototype.redirect = function (code, url) {
code = this[kReplyHasStatusCode] ? this.raw.statusCode : 302
}

this.header('location', url).code(code).send()
return this
return this.header('location', url).code(code).send()
}

Reply.prototype.callNotFound = function () {
Expand Down Expand Up @@ -445,16 +440,9 @@ function onSendEnd (reply, payload) {

// we check if we need to update the trailers header and set it
if (reply[kReplyTrailers] !== null) {
const trailerHeaders = Object.keys(reply[kReplyTrailers])
let header = ''
for (const trailerName of trailerHeaders) {
if (typeof reply[kReplyTrailers][trailerName] !== 'function') continue
header += ' '
header += trailerName
}
// it must be chunked for trailer to work
reply.header('Transfer-Encoding', 'chunked')
reply.header('Trailer', header.trim())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can completely delete filter here (and also in sendTrailer()) because it's already validated in Reply.prototype.trailer(), but if we leave it, then we must also check it for invalid trailers (INVALID_TRAILERS), wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

No, removeTrailer will set the value to undefined. It is required to check if the value is function.

INVALID_TRAILERS is not the same case, as it never be able to set the invalid key-value pair. There is no need to check it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

reply.header('Trailer', Object.keys(reply[kReplyTrailers]).filter(trailer => typeof reply[kReplyTrailers][trailer] === 'function').join(' '))
Copy link
Member

Choose a reason for hiding this comment

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

filter() is very slow.

}

if (payload === undefined || payload === null) {
Expand Down Expand Up @@ -485,10 +473,8 @@ function onSendEnd (reply, payload) {
}

if (reply[kReplyTrailers] === null) {
if (!reply[kReplyHeaders]['content-length']) {
reply[kReplyHeaders]['content-length'] = '' + Buffer.byteLength(payload)
} else if (req.raw.method !== 'HEAD' && reply[kReplyHeaders]['content-length'] !== Buffer.byteLength(payload)) {
reply[kReplyHeaders]['content-length'] = '' + Buffer.byteLength(payload)
denshakhov marked this conversation as resolved.
Show resolved Hide resolved
if (!reply.hasHeader('content-length') || (req.raw.method !== 'HEAD' && parseInt(reply.getHeader('content-length'), 10) !== Buffer.byteLength(payload))) {
Copy link
Member

Choose a reason for hiding this comment

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

please use the fast-path for getHeader.

reply.header('content-length', '' + Buffer.byteLength(payload))
}
}

Expand Down Expand Up @@ -568,12 +554,15 @@ function sendStream (payload, res, reply) {

function sendTrailer (payload, res, reply) {
if (reply[kReplyTrailers] === null) return
const trailerHeaders = Object.keys(reply[kReplyTrailers])

const trailers = {}
for (const trailerName of trailerHeaders) {

for (const trailerName in reply[kReplyTrailers]) {
Eomm marked this conversation as resolved.
Show resolved Hide resolved
if (typeof reply[kReplyTrailers][trailerName] !== 'function') continue

trailers[trailerName] = reply[kReplyTrailers][trailerName](reply, payload)
}

res.addTrailers(trailers)
}

Expand Down