Replacing querystring with fast-querystring

[anonrig]

I've been working on a fast querystring parser for Node.js and saw that Express.js is using the native querystring module. As of right now, fast-querystring is 40% faster than querystring.

express/lib/utils.js

Line 24 in bf4c3ee

var querystring = require('querystring');

Here's an example of a benchmark of Nest.js with replacing fast-querystring and having a 25% op/sec increase with an old version of fast-querystring. - nestjs/nest#10248 (comment)

I'd be happy to open a pull request, but before that, I wanted to create an issue and have a discussion about it.

[dougwilson]

That looks really nice! We'll have to verify the changes between the built in querystring and your module for all the supported Node.js versions if the desire is to include this as a non breaking change. The main question I have though: why not get these improvements in Node.js core? We would prefer to use what is included in Node.js over modules if possible. Ideally I'd like to know if Node.js is rejecting your improvements for some particular reason, as maybe that reason may affect our usgae as well.

[anonrig]

Hey @dougwilson

Thanks for the fastest maintainer response on a large-scale Github repository.

I didn't open an issue or a pull request to Node.js since the querystring module is considered legacy, which means, it is no longer actively maintained and other alternatives are available. For more information: https://nodejs.org/api/documentation.html#stability-index

I have an active open pull request to change this on both Nest.js and find-my-way.

fast-querystring already uses the same test suite from the node.js repository, but lacks testing it on different Node versions. (Note taken. I'll definitely add it to the workflow)

Edit:
I improved the testing suite. Test succeeds for Node 14, 16, 18 on Windows, Linux and macOS. (anonrig/fast-querystring#16)

[dougwilson]

Ah, I see. And as far as compatibility goes, I guess I don't mean testing your module on different Node.js versions, but rather, comparing it to the querystring of different Node.js version. For example, take the scenario of a Express.js user on Node.js 10.0.0. They update Express.js as a non-breaking release (minor version I suppose). Now their req.query is no longer the results of the built-in Node.js querystring parser, but instead fast-querystring. The question is: is that scenario going to result in different behavior of the app? If so, it would be a breaking change for Express.js to release, if that makes sense. My guess based off your readme is that it wouldn't be able to land in Express.js 4.x since some Node.js versions will return the object with a object prototype, but the fast-querystring module only returns with null prototype, breaking the behavior on those apps just by upgrading a minor Express.js (thus would need to be deferred to a major Express.js release).

[anonrig]

That behavior is consistent with the existing querystring module implementation.

Prototype functions are considered harmful since it is open to prototype pollution. Here's the code from the Node.js repository that validates the implementation.

Even though it might be a breaking change (I don't know when Node.js introduced non-prototype objects for querystring), I'm up for fixing this vulnerability for previous node versions.

[dougwilson]

Yes, folks who want that changed can always upgrade their Node.js version or set a custom querystring parser in Express.js and keep using the older Node.js version. That is something up to those apps as they need. Express.js cannot make such a change as a non-breaking release, however, just like Node.js did not.

That was just an example based on your readme. I assume you took the querystring tests from the current Node.js version and none of the past ones. Even if the prototype breaking change was acceptable, we'd still need an enumeration of all the changes an Express.js app would experience if the desire is to try and get this in a 4.x release vs deferring to the next major.

I hope that makes sense. Really, this is just about a discussion for where such a change like this should be landed vs if it's landed at all. I believe there is the desire to move from querystring to the standard URLSearchParams, and this seems like a step in the wrong direction in relation to that, however.

[anonrig]

I see. That's a good point.

I added a new test suite to validate if the existing test suite returns the same result as fast-querystring, and it seems Node 14+ returns non-protototype object for parse.

anonrig/fast-querystring@c77259e

[anonrig]

I did some research and found that querystring.parse did not have a breaking change after Node 8. (https://nodejs.org/api/querystring.html#querystringparsestr-sep-eq-options)

According to test-querystring.js on Node repository's Git blame, all test files are 6+ years old. Except for a fix on stringify which was backported to 12.18.3 (nodejs/node#33918). Since express only uses querystring.parse that's ok.

I believe it's safe to say that if we are aiming for >= 8, there isn't any breaking change on parse and stringify.

[dougwilson]

I believe it's safe to say that if we are aiming for >= 8, there isn't any breaking change on parse and stringify.

Since Express.js 4.x targets Node.js 0.10+, it sounds like this would indeed be breaking for Express.js 4.x and would thus need to land on 5.x, but as I mentioned above, the desire is to move from querystring to the standard URLSearchParams for 5.x, so it does look like there is not a place that we could land fast-querystring in this project.

[anonrig]

I see. FYI, URLSearchParams are also really slow in both parse and stringify. (https://github.com/anonrig/fast-querystring#benchmark)

Thanks for your time. I'm closing this issue.

[dominikg]

@dougwilson with express5 in beta, are you still considering to switch to URLSearchParams? it might not be available on older node versions but could be polyfilled then.