refactor: update Reply module #4072
Conversation
* Use node.js 14 syntax * Improve code readability * Reuse Reply's methods where possible
lib/reply.js
Outdated
| // it must be chunked for trailer to work | ||
| reply.header('Transfer-Encoding', 'chunked') | ||
| reply.header('Trailer', header.trim()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
lib/reply.js
Outdated
| // 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(' ')) |
lib/reply.js
Outdated
| 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))) { |
There was a problem hiding this comment.
please use the fast-path for getHeader.
lib/reply.js
Outdated
| } | ||
|
|
||
| Reply.prototype.removeTrailer = function (key) { | ||
| if (this[kReplyTrailers] === null) return this | ||
| this[kReplyTrailers][key.toLowerCase()] = undefined | ||
| if (this[kReplyTrailers] !== null) { |
There was a problem hiding this comment.
I think I add it as a early exit approach.
The readability didn't change a lot when here is few line of code.
lib/reply.js
Outdated
| for (var i = 0; i !== keys.length; ++i) { | ||
| const key = keys[i] | ||
| this.header(key, headers[key]) | ||
| for (const headerName in headers) { |
There was a problem hiding this comment.
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:
- 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.
lib/reply.js
Outdated
| @@ -165,6 +165,7 @@ 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() | |||
|
|
|||
There was a problem hiding this comment.
What is happening here? Is this a line ending change?
There was a problem hiding this comment.
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?
|
CI is failing |
|
Looks like CI is passing after I merged in the main branch. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Hi, I refactored the Reply module a little bit.
Tests are passing, the benchmark result is the same as before.
Checklist
npm run testandnpm run benchmarkand the Code of conduct