Closed TheDevMinerTV closed 2 months ago
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.
Sounds good, do you want me to make a PR to use the built-in URLSearchParams
?
@TheDevMinerTV if you want to make a PR targeting 5.x, that would be great
Will do ^^ Can you assign me to this issue?
In #4990 it was suggested to replace querystring
(built-in Node module) with URLSearchParams
.
Now Express offers 2 "query parser"
values:
"simple"
- using querystring
, default since 5.0-beta.1"extended"
- using qs
, default in 4.xDoes replacing both of them with URLSearchParams
make sense? Doesn't qs
have more features than URLSearchParams
?
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.
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
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: https://github.com/sindresorhus/got/issues/1559#issuecomment-747350228
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.
sounds like we should continue to use qs
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 ^
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')
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
- https://github.com/expressjs/express/issues/4990#issuecomment-1241219909; PRs welcome), with qs
left as the "extended"
option for compatibility.
Completely removing qs
and the "extended"
option probably would require deprecating it first.
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
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.
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?
No, it will continue to be CJS-only
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.
why is the world talking about node --version these days? what the actual ** is going on
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
Faster-qs supports the requested feature https://github.com/hans00/faster-qs
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.
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 😄
Pros
qs
) vs. ~30 kB (fast-querystring
)qs
's subdeps have a lot of recursion in require cycles as wellexpress
(if there's platform that allowsexpress
to be used, at least)Cons
fast-querystring
requires at least Node 8:fast-querystring
doesn't support setting prototypesNotes
benchmark
folder in this repo and just replacing therequire
call tofast-querystring
on a idling 7950X running Node v8.17.0URLSearchParams
which has been supported since Node 10URLSearchParams
to be pretty slow, though, this needs to be benchmarked first