Investigate replacing qs

[TheDevMinerTV]

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
  • just looked at Replacing querystring with fast-querystring #4990 which mentioned URLSearchParams to be pretty slow, though, this needs to be benchmarked first

[ctcpip]

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]

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

[ctcpip]

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

[TheDevMinerTV]

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

[krzysdz]

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]

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]

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]

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]

sounds like we should continue to use qs

[TheDevMinerTV]

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]

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]

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]

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]

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]

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]

No, it will continue to be CJS-only

[TheDevMinerTV]

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.

[43081j]

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]

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

[wesleytodd]

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]

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

[wesleytodd]

I think the point I was trying to make above is that any movement on this at the moment is not something we are considering. Additional deps is not a concern we have (at least at the moment) and neither is performance (again, at least at this moment). Stability is the primary concern until we can get v5 shipped and v4 EOL'd. Once we have achieved those goals a lot more becomes viable for the project.

If y'all are interested in helping us make those two goals happen first then please join the fun, we need all the help we can get. Until then though, I will be un-following this thread so I can stay focused on what is important for the ecosystem and Express community.

[ctcpip]

just an update, since v5 is now out and is the current version:

• in v5 the default is node:querystring
• users can bring their own parser
• we do bundle qs still, but unless users are opting into it then the only impacts are:
  • module loading, a one-time hit, which is negligible for an express app
  • transitive dependencies
  • bundle size

[ctcpip]

hey folks. don't shoot the messenger here. you asked, I answered to the best of my understanding.

the change to default to simple was made nearly 4 years ago. (but only released recently)

AFAIK, removing qs was never discussed back then, so this doesn't seem like it was a completely deliberate decision to keep it around

[ctcpip]

ref: #3621

[43081j]

i think it was a choice that could've been done in another, better way

an optional peer would've been good. or drop it entirely because its a breaking change anyway

there's not really any reason to keep it around as far as i can tell, either way. the only reason now seems to be that we can't drop it without a new major (but could've in this major)

but it is what it is. in the next major, maybe there's chance we can do it in the way suggested here

[wesleytodd]

To double down a bit on the "past decisions" bit here, we had nearly 10 years of backed-up work to look through. We chose the most impactful things to land and mostly left the rest as had been decided at in the past. Happy to see criticisms of those decisions, but I am not very partial to spending our (folks who are contributing to the project) time fielding requests/issues from folks who are neither contributors or users of express.

---

Now, on to the more important topic: what is next for query parsing in express?

I think we obviously need to land a parser type for URLSearchParams. If someone wants to open that PR please do.

The much more difficult question is if our users need a more feature rich query string parser. Historically they have, which is why qs was there in the first place and was even default until very recently.

Lacking any formal way to collect user feedback, I think that it will be tough to gauge impact without some real work doing outreach. Changing the status quo should take a bit more rigorous decision making than I have seen in this thread, and I am positive the current contributor team does not have the resources to do something like that. So the next best thing we can do is build a convincing case based on our own data.

So far all of the data I see presented in this thread is using small details and missing the big picture. To build a convincing case, here are the things I would need to see:

1. case for install time: construct a realistic project with and without qs (overrides or modified lock files, or whatever method you choose) and measure total cached and un-cached install times
2. case for startup time: same as above (maybe with a loader or something?) to benchmark startup
3. runtime perf: we have a @expressjs/perf-wg (I am helping setup) which is defining our runtime perf goals/tooling/approach, come help us build that out. For now I did a small test and found no convincing data.

I am also sure there are other things I am not thinking of that y'all are (or can with the right support) that we could do in this area. I suggest that nearly all of this is within scope of the Perf WG, so if y'all want to help us measure install time, or startup time, or whatever other measure please join our effort over there.

[43081j]

I'm not sure we should even be focusing on the performance yet

We currently pull in an opinionated dependency for something which isn't enabled by default. If the community starts upgrading express everywhere, presumably this will lead to a decline of people using qs (we will only know by doing the upgrades though over time). If that's the case, it means all those projects have a dead dependency nothing actually uses.

Long before we consider performance, I think we need to think about that.

You're asking for someone to benchmark the project with and without it. Isn't it pretty much impossible that a piece of code loading a module is faster than one that doesn't?

I think the important thing is we're comparing against not having it at all. Not against another library. Any project without the dependency will of course be faster to install and run than one with it (even if it is milliseconds of time).

I'm going to try get the e18e community to start an effort to upgrade express throughout the ecosystem. If that gets going, we should see the problem become more and more obvious as an increasing amount of projects stop using qs.

[43081j]

i believe the way forward is that express no longer ships any query string parser. it defaults to the built in one, and it allows you to pass in your own if you want any more than that.

in many projects, qs is no longer the appropriate choice. many will not use nesting, many want a faster parser, many just have a different preference.

we already made one good step forward by defaulting to node:querystring, but unnecessarily load and install the qs package still. if the default is now to use the built in one, we shouldn't be shipping any package to do the non-default. we should expect users to provide one IMO.

we're likely to move many projects off qs, and this will quickly become dead weight we load in our prod server at runtime for no reason.

this is before considering any performance differences, of which there are many. many projects can see perf gains by using a different library, or no library at all. many don't need nesting at all. the choice is there at least, but we're still forced to install something we don't use or want.

[ctcpip]

personally, I am more convinced by the argument to remove it, than I am by the original argument to replace it with something else

[43081j]

100% - its very easy to bring your own parser if you need anything node itself doesn't provide

we don't need to have an opinionated choice anymore i think

[ctcpip]

some hard data -- express v5 still has but a fraction of usage compared to v4, but we can already see in the wild a ton of deliberate opting in to extended query parsing:

https://sourcegraph.com/search?q=context:global+set%28%27query+parser%27%2C+%27extended%27+count:1100000&patternType=keyword&sm=0

[ljharb]

Very true - but the download numbers indicate the current upper limit of that upgrade effort, which your efforts certainly may shift.

[43081j]

Indeed, I'm confident the downloads and usages will be a different picture when we're further into the upgrade.

Also let's not forget the default is to use the built in parser. People turning the extended one on is meaningless in much of this discussion. If the extended flag didn't exist - they would install a package and pass that in just the same. Which is what we're asking for

How many people turn extended mode on doesn't tell us how many people want a built in parser to do that.

[benmccann]

I think that query is over-counting quite a bit. For example, it's returning results that specifically switch away from extended query parser:

It's probably desirable to wrap it in quotes, which returns a smaller set of results:

https://sourcegraph.com/search?q=context:global+%22set%28%27query+parser%27%2C+%27extended%27%22+count:1100000&patternType=keyword&sm=0

[benmccann]

Actually, I just noticed that it looks like most of the results are from people checking in their node_modules, which is then finding that code inside the express codebase in their node_modules. Please double check that I'm doing this correctly as I'm unfamiliar with sourcegraph, but if I add -file:node_modules it returns only 37 results:

https://sourcegraph.com/search?q=context:global+count:1100000+%22set%28%27query+parser%27%2C+%27extended%27%22+-file:node_modules&patternType=keyword&sm=0

[ctcpip]

yep, thanks for catching that!

[43081j]

some time has passed since the last few discussions of this. i've thought through it again and here's my current opinion/thoughts

some facts:

• express 5.x defaults to node:querystring
• qs is imported regardless of if it is used or not
• body-parser also imports qs regardless of if it is used or not (thus qs is a deep dependency too)
  • afaict this it not customisable yet (and should be! just like express)

we have "extended" mode for the one unsupported use case: nested objects.

also note: what library we use for that is irrelevant, nothing more than an implementation detail (if we ignore CPU perf for now). all of them exist to provide this extra feature that node:querystring doesn't provide itself.

so given all this:

qs is dead weight in the default (hot) path. we install it, load it up, and never use it.

this should be fixed. the obvious options are:

• lazy load it if it is used
  • downside: we still unnecessarily install it in this case, even though we might never load it
• lazy load it as an optional peer dependency if it is used
• remove the "extended" option and require a parser to be passed in

i'd go for the last option since nobody has a reason anymore to require any specific parser. if you do have a reason, let us know!

from all the comments and discussions, it seems the only reason there's currently an opinionated choice is because it was always that way.

a user can very easily write:

app.set('query parser', qs.parse)

// instead of
app.set('query parser', 'extended');

this seems like a truly easy win. we get to drop a deep tree of dependencies and promote smarter dependency choice (in that people are more likely to then look around for an appropriate parser. many, many people do not need a library at all, esp not fqs/qs/other kitchen sink solutions).

TLDR my suggestions

• change body-parser to have extended: Function | boolean instead of extended: boolean
  • drop qs as part of this
  • when true, default to node:querystring the same as express (so this is inconsistent right now, that alone is enough to justify this part of the change)
  • when a function, use it as-is
• change express query parser setting to be Function | boolean
  • drop qs as part of this
  • if true use node:querystring
  • if false don't parse query strings
  • if a function, use as-is