Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Investigate replacing qs #5723

Closed
TheDevMinerTV opened this issue Jun 24, 2024 · 22 comments
Closed

Investigate replacing qs #5723

TheDevMinerTV opened this issue Jun 24, 2024 · 22 comments

Comments

@TheDevMinerTV
Copy link

TheDevMinerTV commented Jun 24, 2024

Pros

  • Less subdependencies and smaller size:
    • ~1MB (qs) vs. ~30 kB (fast-querystring)
    • https://npmgraph.js.org/?q=qs vs https://npmgraph.js.org/?q=fast-querystring
    • Also less bloat (14 deps vs 1 deps)
      • qs's subdeps have a lot of recursion in require cycles as well
      • all of qs and it's dependencies is maintained by the same developer (could be a security concern)
    • Might be useful for serverless folks that use express (if there's platform that allows express to be used, at least)

Cons

  • fast-querystring requires at least Node 8:
    • it uses object destructuring and trailing commas
      • it might be possible to convince the author to remove the trailing commas, then it would be usable with Node 6
    • Node 8 has been out of support for more than 5 years now (support stopped on Jan 1st 2019, according to endoflife.date/nodejs)
    • Node 0.10 is even older, I can't even find an EOL date for it
  • fast-querystring doesn't support setting prototypes
    • this is unsafe either way, though it can probably get released in express v5

Notes

  • Slightly better performance:
    • this test was done using the benchmark folder in this repo and just replacing the require call to fast-querystring on a idling 7950X running Node v8.17.0
  • It might be useful to maybe port to using URLSearchParams which has been supported since Node 10
@ctcpip
Copy link
Member

ctcpip commented Jun 24, 2024

It can't be done in Express 4; as you correctly noted, it would require a higher minimum version of Node. Express 5 will require a minimum of Node 18, and if a dependency is no longer required, I would rather just drop it. However, a significant perf difference could be a compelling enough reason to use it over built-ins.

@TheDevMinerTV
Copy link
Author

Sounds good, do you want me to make a PR to use the built-in URLSearchParams?

@ctcpip
Copy link
Member

ctcpip commented Jun 24, 2024

@TheDevMinerTV if you want to make a PR targeting 5.x, that would be great

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Jun 24, 2024

Will do ^^
Can you assign me to this issue?

@krzysdz
Copy link
Contributor

krzysdz commented Jun 25, 2024

In #4990 it was suggested to replace querystring (built-in Node module) with URLSearchParams.
Now Express offers 2 "query parser" values:

  1. "simple" - using querystring, default since 5.0-beta.1
  2. "extended" - using qs, default in 4.x

Does replacing both of them with URLSearchParams make sense? Doesn't qs have more features than URLSearchParams?

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Jun 25, 2024

URLSearchParams is the standard way of doing query param parsing nowadays. We should probably stick to that, especially since it will work in all runtimes because it's in the ECMAscript URL spec. Though, using node:querystring would also be fine, I guess.

@ctcpip
Copy link
Member

ctcpip commented Jun 25, 2024

your point is valid, but note that URLSearchParams is part of the URL spec, not ECMAScript

node:querystring is not sufficient because it doesn't handle deep nesting

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Jun 25, 2024

URLSearchParams doesn't seem to parse /?foo[0][bar]=baz&foo[0][fizz]=buzz&foo[]=done! into {"foo":[{"bar":"baz","fizz":"buzz"},"done!"]}, but rather as {"foo[0][bar]":"baz","foo[0][fizz]":"buzz","foo[]":"done!"}. So it doesn't support automatic array parsing (which can be worked around, but that might be vulnerable). This was also described here: sindresorhus/got#1559 (comment)

Edit: fast-querystring also doesn't implement this.

Edit: This is very likely because the spec never mentions arrays. I'm not sure how to proceed here. Should we implement (or pull in a package for) the array parsing algorithm, if so, do we use the comma or the index syntax. Or should we drop the feature? IMO we should implement this ourselves with the index syntax.

@ctcpip
Copy link
Member

ctcpip commented Jun 25, 2024

sounds like we should continue to use qs

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Jun 25, 2024

There seems to be picoquery which is ~40KB and seems to support the different modes but seems to fails with parsing on some stuff because it only supports on syntax at a time. I've opened an issue on their repository ^

@dominikg
Copy link

sounds like we should continue to use qs

In the spirit of the very first line of your readme

Fast, unopinionated, minimalist web framework for Node.js.

I believe it would be great to provide a URLSearchParams based interface by default and offer pluggable support for qs or other parsers to allow users to decide for themselves without forcing qs on them.

qs is quire large https://pkg-size.dev/qs and most applications today are just fine with using URLSearchParams on the server and client. It even has basic array support built in new URLSearchParams('foo=1&foo=2').getAll('foo')

@krzysdz
Copy link
Contributor

krzysdz commented Jun 25, 2024

I believe it would be great to provide a URLSearchParams based interface by default and offer pluggable support for qs or other parsers to allow users to decide for themselves without forcing qs on them.

There is a query parser setting, which allows users to configure any query parser they want. Since 5.0 it defaults to "simple" (which may move from node:querystring to URLSearchParams - #4990 (comment); PRs welcome), with qs left as the "extended" option for compatibility.

Completely removing qs and the "extended" option probably would require deprecating it first.

@43081j
Copy link

43081j commented Jun 25, 2024

it probably does make sense that the query parser setting can be simple or a function (in some future breaking version)

since then you can use whatever query parser you like by passing it in, without having to bloat express itself with such a library

@dominikg
Copy link

Another option for 5.0 could be to make qs an optional peerDependency and users opting for the "extended" setting would have to install it manually.

@TheDevMinerTV TheDevMinerTV changed the title Investigate replacing qs with fast-querystring Investigate replacing qs Jun 25, 2024
@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Jun 25, 2024

Removing qs from express, assuming the 29,679,417 downloads / week from NPMJS.com is correct, would save over 75 TB / month (or over ~900 TB / year). body-parser also depends on qs and that's another ~30 million downloads / week.
(correct me if my math is wrong though: https://www.wolframalpha.com/input?i=%2829679417+%2F+7++*+30+*+12%29+*+594+KB)

@ctcpip will express v5 be ESM-only?

@ctcpip
Copy link
Member

ctcpip commented Jun 25, 2024

No, it will continue to be CJS-only

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Jun 25, 2024

Alright, so it we'd have to use picoquery v1.*.*, though I'm willing to make a PR for the project to support both, ESM and CJS, at the same time. @43081j lmk if you want me to work on that.

@zmzlois

This comment was marked as spam.

@43081j
Copy link

43081j commented Jun 25, 2024

Alright, so it we'd have to use picoquery v1.*.*, though I'm willing to make a PR for the project to support both, ESM and CJS, at the same time. @43081j lmk if you want me to work on that.

It was an active choice to keep them separate (1.x cjs, 2.x esm - at the next breaking change, esm only)

We will be backporting non breaking features and fixes to 1.x meanwhile

@anonrig
Copy link

anonrig commented Jul 18, 2024

Faster-qs supports the requested feature https://github.com/hans00/faster-qs

@wesleytodd
Copy link
Member

wesleytodd commented Jul 18, 2024

I dont think we will be replacing this module, at least not any time soon. We are pushing to get v5 our the door and this is not on the list for that. Just want to temper folks expectations since the new group of maintainers, while no longer just one person, is still small and we have a lot of catch up we are doing.

Additionally, with #5731 and v5 landing the default swap I would not be surprised if we went the path of removing all these as direct deps and go to them as 3rd party configuration additions instead for v6. Not promising anything, but also I don't want folks to waste a lot of time bikeshedding this issue for no reason.

@PuruVJ
Copy link

PuruVJ commented Jul 23, 2024

I just made neoqs, fully backward compatible and forever going to follow qs(even in terms of code). It doesn't aim to be faster or drastically smaller. It just aims to do exact same as qs without adding additional deps.

I'd suggest URLSearchParams as the endgame, but in the meanwhile, neoqs could fill in the gap without being disruptive 😄

https://x.com/puruvjdev/status/1815658973946626209

@expressjs expressjs locked and limited conversation to collaborators Jul 23, 2024
@ctcpip ctcpip converted this issue into discussion #5783 Jul 23, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants