refactor: optimize internal read function #587
Conversation
| 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
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! 😊
| 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
wesleytodd,
UlisesGascon,
jonchurch and
bjohansebas
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
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the internal read function to improve maintainability and readability by replacing the legacy "encoding" parameter with a more appropriately named "charset" and inlining decompression stream logic.
- Replaces "encoding" with "charset" consistently in read-related modules.
- Inlines decompression stream logic into the read function and removes the deprecated contentstream helper.
- Updates error messages and function calls to use "charset" instead of "encoding".
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/types/urlencoded.js | Replaces "encoding" with "charset" in read options. |
| lib/types/text.js | Replaces "encoding" with "charset" in read options. |
| lib/types/raw.js | Replaces "encoding: null" with "charset: null" in read options. |
| lib/types/json.js | Replaces "encoding" with "charset" in read options. |
| lib/read.js | Refactors read function to use "charset" and inlines decompression logic. |
Comments suppressed due to low confidence (1)
lib/types/urlencoded.js:93
- Ensure the API documentation is updated to reflect the renaming of the 'encoding' property to 'charset', so that downstream usage is clear.
read(req, res, next, parse, debug, {
|
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! |
This PR aims to improve the maintainability and readability of the internal read function. It also removes some weird parts of that code.