From c544629b928778f6ba8b4f912f49a638db96cf5d Mon Sep 17 00:00:00 2001 From: Den Shakhov Date: Wed, 22 Jun 2022 18:18:03 +0700 Subject: [PATCH 1/5] refactor: update Reply module * Use node.js 14 syntax * Improve code readability * Reuse Reply's methods where possible --- lib/reply.js | 109 +++++++++++++++++++++++---------------------------- 1 file changed, 49 insertions(+), 60 deletions(-) diff --git a/lib/reply.js b/lib/reply.js index c3c3bfc7bf..0d2e70e168 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -135,15 +135,15 @@ Reply.prototype.send = function (payload) { } if (Buffer.isBuffer(payload)) { - if (hasContentType === false) { - this[kReplyHeaders]['content-type'] = CONTENT_TYPE.OCTET + if (!hasContentType) { + 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') { + this.type(CONTENT_TYPE.PLAIN) onSendHook(this, payload) return this } @@ -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')) { + if (!hasContentType) { + 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')) { const customContentType = contentType.trim() + 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`) } } } @@ -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) } Reply.prototype.getHeaders = function () { @@ -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) { @@ -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]) + for (const headerName in headers) { + this.header(headerName, headers[headerName]) } + return this } @@ -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()] } Reply.prototype.removeTrailer = function (key) { - if (this[kReplyTrailers] === null) return this - this[kReplyTrailers][key.toLowerCase()] = undefined + if (this[kReplyTrailers] !== null) { + this[kReplyTrailers][key.toLowerCase()] = undefined + } + return this } @@ -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) } Reply.prototype.redirect = function (code, url) { @@ -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 () { @@ -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()) + reply.header('Trailer', Object.keys(reply[kReplyTrailers]).filter(trailer => typeof reply[kReplyTrailers][trailer] === 'function').join(' ')) } if (payload === undefined || payload === null) { @@ -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) + if (!reply.hasHeader('content-length') || (req.raw.method !== 'HEAD' && parseInt(reply.getHeader('content-length'), 10) !== Buffer.byteLength(payload))) { + reply.header('content-length', '' + Buffer.byteLength(payload)) } } @@ -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]) { if (typeof reply[kReplyTrailers][trailerName] !== 'function') continue + trailers[trailerName] = reply[kReplyTrailers][trailerName](reply, payload) } + res.addTrailers(trailers) } From d1fe724145357f89580f7a809667b4f944644e91 Mon Sep 17 00:00:00 2001 From: Den Shakhov Date: Thu, 23 Jun 2022 19:20:26 +0700 Subject: [PATCH 2/5] refactor: partial revert --- lib/reply.js | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/lib/reply.js b/lib/reply.js index 0d2e70e168..fbd5fed59e 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -135,15 +135,15 @@ Reply.prototype.send = function (payload) { } if (Buffer.isBuffer(payload)) { - if (!hasContentType) { - this.type(CONTENT_TYPE.OCTET) + if (hasContentType === false) { + this[kReplyHeaders]['content-type'] = CONTENT_TYPE.OCTET } onSendHook(this, payload) return this } - if (!hasContentType && typeof payload === 'string') { - this.type(CONTENT_TYPE.PLAIN) + if (hasContentType === false && typeof payload === 'string') { + this[kReplyHeaders]['content-type'] = CONTENT_TYPE.PLAIN onSendHook(this, payload) return this } @@ -157,20 +157,20 @@ Reply.prototype.send = function (payload) { payload = this[kReplySerializer](payload) } - // The 'includes' below also matches custom json mimetypes such as 'application/hal+json' or 'application/ld+json' - } else if (!hasContentType || contentType.includes('json')) { - if (!hasContentType) { - this.type(CONTENT_TYPE.JSON) + // 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 } else { // If user doesn't set charset, we will set charset to utf-8 - if (!contentType.includes('charset')) { + if (contentType.indexOf('charset') === -1) { const customContentType = contentType.trim() if (customContentType.endsWith(';')) { // custom content-type is ended with ';' - this.type(`${customContentType} charset=utf-8`) + this[kReplyHeaders]['content-type'] = `${customContentType} charset=utf-8` } else { - this.type(`${customContentType}; charset=utf-8`) + this[kReplyHeaders]['content-type'] = `${customContentType}; charset=utf-8` } } } @@ -275,13 +275,12 @@ Reply.prototype.trailer = function (key, fn) { } Reply.prototype.hasTrailer = function (key) { - return !!this[kReplyTrailers]?.[key.toLowerCase()] + return this[kReplyTrailers]?.[key.toLowerCase()] !== undefined } Reply.prototype.removeTrailer = function (key) { - if (this[kReplyTrailers] !== null) { - this[kReplyTrailers][key.toLowerCase()] = undefined - } + if (this[kReplyTrailers] === null) return this + this[kReplyTrailers][key.toLowerCase()] = undefined return this } @@ -317,7 +316,8 @@ Reply.prototype.serializer = function (fn) { } Reply.prototype.type = function (type) { - return this.header('content-type', type) + this[kReplyHeaders]['content-type'] = type + return this } Reply.prototype.redirect = function (code, url) { @@ -440,9 +440,16 @@ 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', Object.keys(reply[kReplyTrailers]).filter(trailer => typeof reply[kReplyTrailers][trailer] === 'function').join(' ')) + reply.header('Trailer', header.trim()) } if (payload === undefined || payload === null) { @@ -473,8 +480,8 @@ function onSendEnd (reply, payload) { } if (reply[kReplyTrailers] === null) { - if (!reply.hasHeader('content-length') || (req.raw.method !== 'HEAD' && parseInt(reply.getHeader('content-length'), 10) !== Buffer.byteLength(payload))) { - reply.header('content-length', '' + Buffer.byteLength(payload)) + if (!reply[kReplyHeaders]['content-length'] || (req.raw.method !== 'HEAD' && parseInt(reply[kReplyHeaders]['content-length'], 10) !== Buffer.byteLength(payload))) { + reply[kReplyHeaders]['content-length'] = '' + Buffer.byteLength(payload) } } From 4573e1cc48ce824f865cdd8659c4623b2d9227cf Mon Sep 17 00:00:00 2001 From: Den Shakhov Date: Thu, 23 Jun 2022 19:55:02 +0700 Subject: [PATCH 3/5] refactor: revert for in loop --- lib/reply.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/reply.js b/lib/reply.js index fbd5fed59e..eb87f59a53 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -233,8 +233,11 @@ Reply.prototype.header = function (key, value = '') { } Reply.prototype.headers = function (headers) { - for (const headerName in headers) { - this.header(headerName, headers[headerName]) + 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]) } return this From 6602564e0de9e39804d337a53afd6130db7ce013 Mon Sep 17 00:00:00 2001 From: Den Shakhov Date: Mon, 27 Jun 2022 20:42:23 +0700 Subject: [PATCH 4/5] refactor: if condition --- lib/reply.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/reply.js b/lib/reply.js index eb87f59a53..ad591aa3c8 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -483,7 +483,12 @@ function onSendEnd (reply, payload) { } if (reply[kReplyTrailers] === null) { - if (!reply[kReplyHeaders]['content-length'] || (req.raw.method !== 'HEAD' && parseInt(reply[kReplyHeaders]['content-length'], 10) !== Buffer.byteLength(payload))) { + const contentLength = reply[kReplyHeaders]['content-length'] + if (!contentLength || + (req.raw.method !== 'HEAD' && + parseInt(contentLength, 10) !== Buffer.byteLength(payload) + ) + ) { reply[kReplyHeaders]['content-length'] = '' + Buffer.byteLength(payload) } } From 7b515d10f35b995dd1ddf66da4e2a299d5b85acb Mon Sep 17 00:00:00 2001 From: Den Shakhov Date: Mon, 27 Jun 2022 21:12:06 +0700 Subject: [PATCH 5/5] refactor: partial revert --- lib/reply.js | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/lib/reply.js b/lib/reply.js index ad591aa3c8..45db78b96e 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -165,7 +165,6 @@ Reply.prototype.send = function (payload) { // If user doesn't set charset, we will set charset to utf-8 if (contentType.indexOf('charset') === -1) { const customContentType = contentType.trim() - if (customContentType.endsWith(';')) { // custom content-type is ended with ';' this[kReplyHeaders]['content-type'] = `${customContentType} charset=utf-8` @@ -187,8 +186,12 @@ Reply.prototype.send = function (payload) { Reply.prototype.getHeader = function (key) { key = key.toLowerCase() - - return this[kReplyHeaders][key] ?? this.raw.getHeader(key) + const res = this.raw + let value = this[kReplyHeaders][key] + if (value === undefined && res.hasHeader(key)) { + value = res.getHeader(key) + } + return value } Reply.prototype.getHeaders = function () { @@ -262,18 +265,14 @@ 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) } - - this[kReplyTrailers] = this[kReplyTrailers] ?? {} + if (this[kReplyTrailers] === null) this[kReplyTrailers] = {} this[kReplyTrailers][key] = fn - return this } @@ -284,7 +283,6 @@ Reply.prototype.hasTrailer = function (key) { Reply.prototype.removeTrailer = function (key) { if (this[kReplyTrailers] === null) return this this[kReplyTrailers][key.toLowerCase()] = undefined - return this } @@ -569,15 +567,12 @@ 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 in reply[kReplyTrailers]) { + for (const trailerName of trailerHeaders) { if (typeof reply[kReplyTrailers][trailerName] !== 'function') continue - trailers[trailerName] = reply[kReplyTrailers][trailerName](reply, payload) } - res.addTrailers(trailers) }