Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: require an extended body parser #532

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

43081j
Copy link

@43081j 43081j commented Aug 11, 2024

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

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:

```ts
{
  "extended": (bodyStr) => {
    const params = new URLSearchParams(bodyStr);
    return convertURLSearchParamsToObject(params);
  }
}
```
@wesleytodd
Copy link
Member

wesleytodd commented Aug 13, 2024

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.

@43081j
Copy link
Author

43081j commented Aug 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants