rebase: Generic Body Parser implemented

[ctcpip]

• rebases Generic Body Parser implemented #282
  • targeting the 2.x branch
• fixes object merge logic
• drops CI support for node <4
• configures node 8 and 9 runs to use older versions of dependencies (matches v7 config) due to nyc error

[ctcpip]

cc @sdellysse @jonchurch @wesleytodd @inigomarquinez

[wesleytodd]

Thanks! I think this one is a bit hard to follow just because it is doing a lot at once. The work needs to be done I am nearly certain one way or another, but reading it in this form to make sure we have something that makes sense together is a bit difficult. Is there any easier smaller chunks we could piece out and land to make this refactor easier to follow?

[sdellysse]

@ctcpip do you consider this the "successor PR" to #282 ? If so, I'll close mine with a comment pointing people here

[ctcpip]

@ctcpip do you consider this the "successor PR" to #282 ? If so, I'll close mine with a comment pointing people here

@sdellysse yes

[ctcpip]

Is there any easier smaller chunks we could piece out and land to make this refactor easier to follow?

@wesleytodd I suggest:

• use the split diff view rather than unified
• the substantive changes are:
  • lib
    • generic-parser.js
  • types
    • json.js
    • raw.js
    • text.js
    • urlencoded.js

Before scrutinizing anything in-depth, it helps to do a quick once-over of the changes to get a feel for it:

1. first take a look at raw.js
   • note how there is very little code remaining and much is removed
2. now take a look at generic-parser.js
   • notice that much of the code that was removed from raw.js has moved here
3. now take a look at text.js
   • notice that it looks an awful lot like raw.js

You'll notice the same for the remaining parser types json.js and urlencoded.js. Most of the code from these parsers was deduplicated into generic-parser.js which forms the base for the other parsers. The specific parsers now only contain the logic necessary for their specific cases beyond what is already provided by the generic parser.

My hope is that at this point, things are clear enough to proceed. If not, an alternate way of chunking this out would be by considering each parser refactor in isolation. So:

• json.js -> json.js + generic-parser.js
• raw.js -> raw.js + generic-parser.js
• ...etc.

You could also look at the individual commits such as added generic parser and converted json parser to use generic parser, etc.

The remaining files we didn't talk about are the test files (the changes of which are small enough), and the GH workflow files, where scorecard was just pulled from mainline, and CI was just updated to remove unsupported node versions and fix dependency issues with 8 and 9.

[inigomarquinez]

Hi @ctcpip !

Thanks for the contribution!

Here my 2 cents:

• Related to configures node 8 and 9 runs to use older versions of dependencies (matches v7 config) due to nyc error, there is already a PR for that. I suggest to merge that PR first so you don't need to deal with it in yours and as @wesleytodd suggests not doing a lot at once.

• Related to drops CI support for node <4, I'm not sure if we want to drop that support. @carpasse and I have been migrating a lot of repositories to use GitHub actions instead of travis and in the migration we have kept those versions too for express v4 (express v5 will support only from node >=18). So I would keep those versions in the CI.

[wesleytodd]

Related to drops CI support for node <4, I'm not sure if we want to drop that support.

I agree with this in general, but we were looking at it and it appears this package already dropped those versions at one point, so it might be that this change is just to reflect that it was already released as dropping those node versions.

[ctcpip]

this PR is meant to land in body-parser v2 and is thus targeting the 2.x branch. the PR for 2.x to merge to master lists dropping support for node below 4.

separately, we should still merge the existing CI PR -- (not sure what's the status with the test failure there). we can rebase 2.x and this branch as needed.

as for whether this branch should include any CI changes at all, if the substantive changes of this PR are good to go, I think it's probably fine to keep them together as they are separate commits and both need to go to 2.x anyway. but if folks want, I can split that off into a separate PR and merge that to 2.x and then rebase this branch off that

[wesleytodd]

I just landed the CI change for 2.x (it just needed to be done so I didn't see a reason to wait on approvals). Do we want to rebase this again and remove the CI changes so that we can run it through the updated CI?

[wesleytodd]

@jonchurch I talked with @ctcpip and he was alright if we decided to punt on this until 3.x (and thus 6.x for express). I wanted to check if you are alright with that. I have one remaining item on #66 that I hope to have resolved asap so this is the only remaining one in question. If we are alright delaying more on this I can cut 3.0.0 and update the v5 branch with it which crosses one of the last major remaining todo items off the list.

[ctcpip]

deleting the 2.x branch had the effect of closing this PR. #544 was opened to replace it

[UlisesGascon]

deleting the 2.x branch had the effect of closing this PR

🤦‍♂️ I forgot about this pr