Generic Body Parser implemented

[sdellysse]

Discussion in #281 led to this PR. This PR refactors all existing parsers, extracting common parsing functionality into a generic-parser, which the parser are built on top of. This makes adding and maintaining new parsers relatively trivial.

All tests pass.

PR #281 was referencing issue #278 and issue #42. Issue #22 was suggest as a whole fix. This PR implements #22's solution, but also lets you supply your own parser to both json and urlencoded parsers because the setup (even with the generic parser extracted) is non trivial.

[sdellysse]

Due to the scope of this change, the diff isn't very helpful. It's probably best to just read the individual files themselves.

[dougwilson]

This is super great! I took a quick look through it the other day, and will take a closer look tomorrow with the intent to land as soon as possible 👍 . The CI is failing on Node.js 0.8 right now. I haven't looked yet as to why that is, but I can land that in 2.0 since that won't support Node.js 0.8 anyway.

[sdellysse]

Thanks Doug! Yeah, I noticed that CI check failing, it looks like the object-assign module doesn't work on that version of node, which is a trivial bit that can be re-written if need be.

Obviously, the biggest worry with this is breaking backwards compatibility, but it appears to be a non-issue if the test suites are to be trusted. If we can feel confident that we won't break the world this might even be able to be merged with only a minor version bump.

Looking back over this the one thing that needs to still be done that I overlooked is documentation for the genericParser, as in how to implement one.

[chemarhs]

Lo necesitó , Gracias.

[bdharrington7]

Hi, Is there an ETA on when this can be published? This is great @shawndellysse! The timing couldn't be better :)

[dougwilson]

Unless it is updated to pass the current CI, it has to wait until 2.0 to be published.

[stevebeem]

@shawndellysse Any idea when this will land? Looking forward to using this with json-bigint as soon as possible.
Thank you for taking the time to implement this!

[sdellysse]

@dougwilson I know it's been a while but is this something that is still of usefulness? This came up in a conversation I was having and I came back to check on it to find it's still open.

Obviously, it would have to updated to the current codebase, but I was just wanting to make sure that it's something that would be useful to the project if I took the time to do that.

[dougwilson]

Yes, it is still useful. In the coming days I am going to get the 2.0 branch up to date on master as well, so if you wanted to rebase this onto current master that would be a huuuuge help :)

This is because I assumed that since I never heard back about making it pass CI you wanted to hold this for 2.0

[sdellysse]

Ok, I rebased my branch on to current master, updated the code to satisfy the current tests and linters.

I removed the dependency on object-assign, which didn't support node versions < 0.10.

I had to add a suppress-global-leak flag to the npm test command, due to it being babel's expected behaviour (source: babel/babel-loader#152) on older versions of node.

[sdellysse]

@dougwilson is there anything else I need to do to get this added?

[dougwilson]

Hi @sdellysse ; I saw your pushes over the weekend, but haven't gotten back to take a look around. So AFAIK I don't think so, but don't hold me to that :) I just need to take a second look. If anything does need changed, I can also help make the changes as well if needed.

[EdgBuc]

Hello,
thank you @sdellysse for nice effort!
Is this planned to be merged in the near future? :)

[dougwilson]

Yes, that is absolutely the plan.

[andidev]

@dougwilson it has been a while. Any plans on merging this soon?

[dougwilson]

Hi @andidev yes, indeed. It kind of fell to the way side, but I just don't want to land it as-is, as it does at least three different things within the single PR: adds a generic parser, adds a parser option for json parser, and adds a parser option for urlencoded parser. I started work to split this into those three so I can last the first of the three, but never completed it. I may try to finish that over this holiday weekend that is coming up (in the US). The two parser options do not seem very useful and, especially in the case of urlencoded, are not ergonomic (depending on the value of one option provided, others may be ignored, for example). The parser on the json one seems fine, but when I looked at the implementation it just adds complication to the existing json parser when it seems just as easy to use the generic parser directly.

[dougwilson]

Sorry, and I forgot to add (after pulling up my notes) that the PR is missing documentation and tests around the added bodyParser.generic as well, so I was going to help create those; we don't want to land functionality without tests/docs, of course :) Without docs it may as well not even be there, as no one will know how to use it, haha.

[tlaukkan]

Looking forward to this getting merged. It would be useful to be able to pass bigint through body-parser.

[sdellysse]

@dougwilson is there any interest in getting this up to date and merged in? I have some free time so I thought to come back to this and see if it was still in pr-hell haha. Is the basic concept something that is agreeable to the project?

[dougwilson]

Yes, very interested. Right ar this moment the Express project is focusing on some other items so may still be some time until it is looked at again, but if you would like to bring it up to date that would be a huge help for when we circle back around to it.

[sdellysse]

closed; successor PR is #524