feat: align the rest of the ContentTypeParser methods with .add#5345
feat: align the rest of the ContentTypeParser methods with .add#5345gurgunday wants to merge 14 commits intofastify:nextfrom
.add#5345Conversation
lib/contentTypeParser.js
Outdated
| } | ||
|
|
||
| ContentTypeParser.prototype.getParser = function (contentType) { | ||
| contentType = contentType.trim() |
There was a problem hiding this comment.
From my point of view, .trim should place way lower place and it should be another string.
So, it would cache the original header and utilize the cache.
There was a problem hiding this comment.
This is an internal method to match the content type and seems like Content-Type is matched correctly even if there are surrounding spaces - so trimming already happens somewhere
As a result, I removed any behavioral changes from it, focusing on the public API
|
Ok everything is much better/simpler now - note that getParser always takes a string parameter Please take a look |
This reverts commit c63b5fc.
|
Benchmark isn't working on |
| const result = parser.fn(request, request[kRequestPayloadStream], done) | ||
|
|
||
| if (result && typeof result.then === 'function') { | ||
| if (typeof result?.then === 'function') { |
There was a problem hiding this comment.
i dont think that this is faster.
There was a problem hiding this comment.
It's not slower either, it just reads better imo, do you disagree?
There is no string comparison as I mentioned previously
| const contentLength = request.headers['content-length'] === undefined | ||
| ? NaN | ||
| : Number(request.headers['content-length']) | ||
| const contentLength = Number(request.headers['content-length']) |
There was a problem hiding this comment.
isNaN(Number(undefined)) === true
That check does nothing
metcoder95
left a comment
There was a problem hiding this comment.
LGTM, but I'd prefer to wait for @climba03003's review
|
I need to test it when I have time. |
Please do let me know when you find it 😁 |
|
@climba03003 I think node does it since this returns the trimmed version here: fastify.post('/', (req, reply) => {
console.log(req.raw.header['content-type'])
reply.send(req.body)
}) |
|
I've found a bug with the I will be concentrating on v5 for a few days |
|
Merged at #5372 |
|
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. |
Per @climba03003's comment, #4477 (comment)
We now trim and lowercase the values of user-added ContentTypeParsers so that weird scenarios like this are not possible
The same behavior should be exhibited by
.removeand.hasParserfor consistencyAfter the merge, follow-up PRs:
hasParserlike the other methods — feat: rework contentTypeParser methods #5372