v5.0.0 Major Release Proposal

[MarkHerhold]

Let's discuss changes for koa-body v5.0.0. These are the items that I'd like to see rolled into the next release.

• Update co-body to 6.0.0
• Modernize koa-body codebase (Test Refactor #136, feature/rework to typescript remove deprecated strict option #213)
• Add linting, coverage, and benchmarks (Test Refactor #136, feature/rework to typescript remove deprecated strict option #213)
• Remove strict option (Remove opts.strict compatibility shim #137)
• Rework options
  • Remove patchNode and patchKoa options ❓
  • Cleaner limits options
• Make onError more predictable (set onError function will get error:'next() called multiple times' #112)
• Drop support for Node.js 6
• Start testing on Node.js 12
• Parse bodies on DELETE by default (Ignoring request body in DELETE requests #163)

I think we'll target late April or early May for this release assuming maintainers and contributors have enough time.

I'm looking forward to hearing thoughts and feedback on this list. 👋

[katanacrimson]

May be worthwhile to investigate testing of the typings, unless it's worthwhile to just go full-bore typescript with koa-body (but that's easily rewrite territory).

Some cleanup may be in order, as well. For example, unparsed.js is a file that only exports a single symbol; we ought to be considering either exporting a map instead or gluing the symbol to the prototype of requestbody before it's exported. We're inconsistent in how we handle opts.onError (we do a typeof check here) versus opts.onFileBegin. The coercion into an array within formy() is a bit messy as well and could use another look.

[katanacrimson]

Carrying something over from #140 - potentially worth looking at for 5.0.0 are what options are actually valuable still (an extension of the bullet point for possibly removing patchNode / patchKoa) and cleaning up the options themselves.

Of note, we have toggles for parsing bodies depending on type - and then we have differently prefixed options for handling size constraints for the bodies as well. We do not, however, have a direct way to control multipart body size limits (both fieldsize and upload filesize limits) and instead kick the user to control those via the formidible property object. That's not a good design, and is a bit orthagonal to how the rest of the body types are treated.

Here's what I'm thinking. We break up the opts for each "body" type into subobjects, and then from there have the properties for configuring behavior for that body type (e.g. enable/disable, limits, potentially more like a mimetype array depending on if we want to address #72...)

ex:

app.use(koaBody({
  json: {
    enabled: false
  },
  url: {
    enabled: true,
    limit: '56k'
  }
})

That also addresses the problem we're going to start developing if we have to keep prefixing options for certain body types.

[katanacrimson]

Care to open a 5.0.0 branch to target PRs toward?

[MarkHerhold]

@damianb created branch v5 - https://github.com/dlau/koa-body/tree/v5

[katanacrimson]

Alright, I'll target it with a PR to handle the removal of the strict shim.

[MarkHerhold]

@damianb be sure to pull from v5. I linted the entire codebase (and broke the coverage check) so I think we're in a good place to be making changes now.

[ZWkang]

oh, i find somethings.
i saw the build test fail,then i saw some code not to be cover in the test case when i want to cover all the test case in the code. i find something maybe problems or maybe nothing

1. bodyPromise.catch().then() should be bodyPromise.then().catch()?

const bodyPromise = Promise.reject(123)
bodyPromise.catch((b) => {
  console.log(b);
  return b
}).then(v => console.log(v))
// run catch and then
bodyPromise.then((v) => {
  console.log(v);
  return v;
}).then(val => console.log(val + value))
// just run catch

beacuse the code then and catch use next() and the koa do not allow next() called multiple times.
so it will get wrong case . maybe it's a bug ?

2. try catch the co-body parse.
   i think co-body parse error will handle with promise chain?
   try catch will be not necessary？

[MarkHerhold]

@ZWkang Please see #59 for a potential explanation

[katanacrimson]

Due to #144 I'm really thinking we need to ditch options.patchKoa, possibly options.patchNode. As noted in that issue, the twin branching paths of patchKoa are creating uncertainty in typings where we honestly should be able to count on at least the body object being defined or even potentially being a Record<string, any>.

I can't help but wonder how many people actually use options.patchKoa = false.

Edit: It appears to have originated all the way back in 2014, and I can't find any request that drove having the patchNode/patchKoa toggles themselves. Weird.

[katanacrimson]

@MarkHerhold Considering we're inherently requiring node 8 now, how do you feel about async/await being used internally?

[MarkHerhold]

@damianb I'd love to switch over to exclusively async/await and see little reason not to.
My only concern is not breaking the fix in this PR with a rewrite. We should probably add a test.

[katanacrimson]

Good point - that definitely needs a test case.

[katanacrimson]

Added a test for what you mentioned in #149.

[katanacrimson]

We probably need to see if co-body intends to merge/release a fix for this: cojs/co-body#70

[merfais]

@ZWkang Please see #59 for a potential explanation

do not call next() in catch can fix this bug? becuase it will be called in then chain if use onError callback.