body-parser icon indicating copy to clipboard operation
body-parser copied to clipboard

feat: require an extended body parser

Open 43081j opened this issue 1 year ago • 2 comments

This changes extended to be settable to a custom body parser, or false.

When false, the simple parser (node:querystring) will be used which supports the following syntax:

  • foo=a&foo=b becomes {"foo":["a","b"]}
  • foo=bar becomes {"foo":"bar"}

Otherwise, it must be a function which parses the given string:

{
  "extended": (bodyStr) => {
    const params = new URLSearchParams(bodyStr);
    return convertURLSearchParamsToObject(params);
  }
}

This way the library no longer enforces a specific query string parser, but instead pushes the user to bring their own.

Most users will be fine with URLSearchParams, so it may make sense for us to implement a very basic conversion of that to a plain object (like in the example above).

If anyone wants more than that, they can bring their own library and pass it in.

Notes:

  • I don't think it makes sense for this to repurpose extended. Rather we would introduce something else I think, like bodyParser
  • I would do a very basic URLSearchParams to object parse function which becomes the default, or just use node:querystring (no object nesting)

cc @wesleytodd this is what i was trying to explain in my comments elsewhere in the other PR. i don't think body-parser should be shipping with a query string parser since it may not always be enabled (so would be a waste). I think we should have a very basic nested parser or none at all

i did this to explain to you what was in my head. if its the totally opposite direction of whats in yours, please feel free to discard the PR. i'll leave it as draft meanwhile

43081j avatar Aug 11 '24 22:08 43081j

Thanks for this, it helps clarify for sure. That said, I am pretty bullish on moving these kind of libraries into the platform. It is both one of the stated goals of the Node.js Web Server Frameworks Team and our long term vision for express that more of these common requirements are shared and supported via the runtime not each server framework group.

So while I agree this is a good general technical decision I hesitate to force end users to go through this change if we are just going to ask them to do another breaking change in the future to move to platform supported extended query string parsing.

To me this needs some more discussion so we can have a clear "north star" goal to work toward here. If it happens that this helps get us there, we can totally merge it, but I want to know what that end goal looks like before making decisions here.

wesleytodd avatar Aug 13 '24 17:08 wesleytodd

If your hope is that node ships a query string parser capable of nesting, it is very unlikely it would have the behaviour qs has (since it's such a mixed bag of behaviours without any standard)

So I think even if you keep this as is, and node ships that one day, you'll be asking users to change the behaviour of their apps.

The only way you would avoid that is by having a qs-like layer around node's parser (if it existed)

If you do that, you're then constrained by a very loosely defined third party API (qs) rather than shaping around what is in the platform

43081j avatar Aug 13 '24 23:08 43081j

I just ran into a need for this. I'd love to see it move forward. I agree with @43081j that I don't see a path for Node to adopt the logic that lives in qs as that would then diverge from how URLSearchParams works in the browser

benmccann avatar Jun 24 '25 19:06 benmccann