Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: require an extended body parser#532

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
43081j wants to merge1 commit intoexpressjs:master
base:master
Choose a base branch
Loading
from43081j:noqs

Conversation

@43081j
Copy link

@43081j43081j commentedAug 11, 2024
edited
Loading

This changesextended to be settable to a custom body parser, orfalse.

Whenfalse, 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)=>{constparams=newURLSearchParams(bodyStr);returnconvertURLSearchParamsToObject(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 withURLSearchParams, 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 repurposeextended. Rather we would introduce something else I think, likebodyParser
  • I would do a very basicURLSearchParams to object parse function which becomes the default, or just usenode: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 thinkbody-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 whichsupports 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 commentedAug 13, 2024
edited
Loading

Thanks for this, it helps clarify for sure. That said, I am pretty bullish on moving these kind of librariesinto 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 commentedAug 13, 2024
edited
Loading

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

@benmccann
Copy link

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 inqs as that would then diverge from howURLSearchParams works in the browser

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@43081j@wesleytodd@benmccann

[8]ページ先頭

©2009-2025 Movatter.jp