refactor: optimize internal read function#587
refactor: optimize internal read function#587Phillip9587 wants to merge 1 commit intoexpressjs:masterfrom
Conversation
lib/read.js
Outdated
| stream.length = undefined | ||
| } catch (err) { | ||
| return next(err) | ||
| const charset = options.charset |
There was a problem hiding this comment.
I renamed the option to charset because it holds the content-type header charset (Content-Type: text/plain; charset=utf-8) or the default charset
| /** | ||
| * Get the content stream of the request. | ||
| * | ||
| * @param {object} req | ||
| * @param {function} debug | ||
| * @param {boolean} [inflate=true] | ||
| * @return {object} | ||
| * @api private | ||
| */ | ||
|
|
||
| function contentstream (req, debug, inflate) { | ||
| var encoding = (req.headers['content-encoding'] || 'identity').toLowerCase() | ||
| var length = req.headers['content-length'] | ||
|
|
||
| debug('content-encoding "%s"', encoding) | ||
|
|
||
| if (inflate === false && encoding !== 'identity') { | ||
| throw createError(415, 'content encoding unsupported', { | ||
| encoding: encoding, | ||
| type: 'encoding.unsupported' | ||
| }) | ||
| } | ||
|
|
||
| if (encoding === 'identity') { | ||
| req.length = length | ||
| return req | ||
| } | ||
|
|
||
| var stream = createDecompressionStream(encoding, debug) | ||
| req.pipe(stream) | ||
| return stream | ||
| } |
There was a problem hiding this comment.
This can be seen as follow-up to #564. I included the contentstream function in the main read function for multiple reasons:
- to prevent the
reqobject from being altered by settinglengthand resetting it inread. - This also improves code by only setting the
raw-bodylengthoption and thus also only callingreq.headers['content-length']when the requests needs no inflation.
| if (contentEncoding === 'identity') { | ||
| // set raw-body expected length | ||
| stream = req | ||
| options.length = req.headers['content-length'] |
There was a problem hiding this comment.
Improves the code by only accessing req.headers['content-length'] and setting the length option for raw-body when the stream needs no decompression
Phillip9587
left a comment
Phillip9587
left a comment
There was a problem hiding this comment.
I’ve added comments to explain my changes. The diffs don’t fully capture the modifications clearly, so I’d recommend comparing the file from master with the version in this branch for better context.
Let me know if you have any questions or need further clarifications. Looking forward to your feedback! 😊
lib/read.js
Outdated
| var encoding = opts.encoding !== null | ||
| ? opts.encoding | ||
| : null |
There was a problem hiding this comment.
I think we can remove that 😅
Phillip9587
requested review from
UlisesGascon,
bjohansebas,
jonchurch and
wesleytodd
Phillip9587
force-pushed
the
perf-content-length
branch
2 times, most recently
from
c0d4f4f to
9a2e3ca
Compare
Phillip9587
force-pushed
the
perf-content-length
branch
from
9a2e3ca to
644df75
Compare
Phillip9587
force-pushed
the
perf-content-length
branch
from
644df75 to
0d5f462
Compare
|
Hey @UlisesGascon @jonchurch, just circling back on this PR - it's been a little while, so I wanted to check in and see if you had a chance to take a look. Let me know if there's anything I should address or update to help move it forward. Thanks! |
|
Hey @UlisesGascon @jonchurch 👋 |
Phillip9587
force-pushed
the
perf-content-length
branch
from
0d5f462 to
9871e65
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the internal read function to improve maintainability and readability by inlining the contentstream helper function and renaming variables for clarity.
- Renamed
encodingvariable tocharsetfor better semantic clarity - Inlined the
contentstreamfunction directly into the mainreadfunction - Simplified variable declarations and moved content-encoding handling earlier in the flow
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (opts.encoding === null && encoding !== null && !iconv.encodingExists(encoding)) { | ||
| return next(createError(415, 'unsupported charset "' + encoding.toUpperCase() + '"', { | ||
| charset: encoding.toLowerCase(), | ||
| if (options.verify && charset !== null && !iconv.encodingExists(charset)) { |
There was a problem hiding this comment.
The charset validation logic has changed incorrectly. Previously, this check was performed when opts.encoding === null (which occurred when verify was truthy), but now it's only performed when options.verify is truthy AND charset is not null. This could skip charset validation when verify is falsy but charset needs to be validated for decoding.
| if (options.verify && charset !== null && !iconv.encodingExists(charset)) { | |
| if (charset !== null && !iconv.encodingExists(charset)) { |
1 participant
This PR aims to improve the maintainability and readability of the internal read function. It also removes some weird parts of that code.