Conversation
wesleytodd
left a comment
wesleytodd
left a comment
There was a problem hiding this comment.
Awesome work getting this back into shape. I am for sure a 👍 on this, but had nits and wanted to agree with your comments about proposed changes. Before a final review time IMO we should land those changes as well but lets maybe give it a bit for other folks like Linus and Chris to have time to review before wasting time if they disagree on those changes.
|
@wesleytodd @ctcpip @jonchurch I’ve addressed the feedback from earlier comments. Please let me know if there’s anything else that needs improvement, but I believe it’s ready for review now. Thanks! |
|
Hi @wesleytodd, @LinusU, @jonchurch, I hope you're doing well! I wanted to kindly follow up and ask if there's any chance we could move forward with this PR. Let me know if there's anything needed on my end to help move things along. |
UlisesGascon
left a comment
UlisesGascon
left a comment
There was a problem hiding this comment.
Awesome work @Phillip9587! ❤️
package.json
Outdated
| "test": "mocha --require test/support/env --reporter spec --check-leaks --bail test/", | ||
| "test-ci": "nyc --reporter=lcov --reporter=text npm test", | ||
| "test-cov": "nyc --reporter=html --reporter=text npm test", | ||
| "test-debug": "npm test -- --timeout 0" |
There was a problem hiding this comment.
I thought it was only added for debugging purposes. I can add it back if you want
ctcpip
left a comment
ctcpip
left a comment
There was a problem hiding this comment.
sorry, I know you spent some good time on this, but I would like to see the rebase commits cleanly applied so that git history makes sense. there are two or three commits after the rebase that should not be necessary
…arsers should import genParser'
|
@ctcpip I rebased it again and removed the two unnecessary commits. Let me know if you want me to readd the |
Phillip9587
left a comment
Phillip9587
left a comment
There was a problem hiding this comment.
Just two minor things I found during the rebase. Should I handle them now or in a follow up PR?
|
|
||
| function createBodyParser (parse, options, defaultOptions) { | ||
| // Squash the options and the overrides down into one object | ||
| var opts = { ...defaultOptions || {}, ...options } |
There was a problem hiding this comment.
| var opts = { ...defaultOptions || {}, ...options } | |
| var opts = { ...defaultOptions, ...options } |
| var bytes = require('bytes') | ||
| var contentType = require('content-type') | ||
| var createError = require('http-errors') | ||
| var debug = require('debug')('body-parser:generic') |
There was a problem hiding this comment.
I suggest passing the debug function as a parameter to createBodyParser(). This will allows the created middlewares to use their namespaces, such as body-parser:json, for debug messages.
@ctcpip Could you please specify which commits you'd like me to remove? |
|
@Phillip9587 Please see my comment in the other PR |
|
clean squash and rebase here: #574 |
5 participants
Rebase of #544 as alternative to #550 and #551
cc @wesleytodd @ctcpip