Replace node:querystring with URLSearchParams

[TheDevMinerTV]

As discussed in #5723, I've replaced node:querystring with URLSearchParams for better interoperability and standards compliance.

The "multiple keys" scenario isn't defined by the URL and URLSearchParams spec so the JSONP test for checking that the first callback parameter is used, is failing.
You can read more here: https://www.rfc-editor.org/rfc/rfc3986#section-3.4 https://url.spec.whatwg.org/#urlsearchparams

❔ How do we want to proceed with this?

Supersedes #5730

cc: @ctcpip

[ctcpip]

There is much that is not defined in specifications for various reasons. JSONP itself was never formalized, for example. But this isn't very relevant here.

The problem at hand isn't so much which key to parse first -- although express does guarantee the order and we wouldn't want to change that. The bigger issue with your change is actually that you're discarding data.

The result of parsing 'callback=something&callback=somethingelse' should result in:

{ callback: [ 'something', 'somethingelse' ] }

but with your change, the result is:

{callback: 'somethingelse'}

Storing duplicate key query string values as an ordered list is standardized in the URL spec and is consistent across query string parser impls in the JS and Web API ecosystems. It is done this way in node, qs, fast-querystring, URLSearchParams, and others.

> new URLSearchParams('callback=something&callback=somethingelse').getAll('callback');
  (2) ['something', 'somethingelse']

[TheDevMinerTV]

I've fixed the USP parser, I just had to manually convert all keys to a POJO using .get(), instead of using Object.fromEntries(). The tests now pass without issues.

[wesleytodd]

Question: is there any value in adding this as an additional option instead of removing the old option entirely?

[TheDevMinerTV]

Question: is there any value in adding this as an additional option instead of removing the old option entirely?

I don't think so since both options should parse the options the same already (sort of). We'd also decouple from Node a bit.

[wesleytodd]

(sort of).

Exactly my concern when raising this question. Is there a downside to keeping the old behavior around? The only thing I could think of is loading in the extra lib, but frankly unless it can be proven to make a meaningful difference in startup time I am inclined to take that penalty so that we avoid increased issue volume on the project.

We'd also decouple from Node a bit.

This is a non-goal of the project right now. Maybe someday when we are not monkey patching node built ins it could be, but I am not confident we will get to a place where express itself is runtime agnostic. I do think we can have most of the components be, like how the router runs just fine in the browser, but I don't think that is a goal perse.

---

In general I would prefer to land a new option to use URLSearchParams to even the v4 branch (and merge that up to v5) and then just swap the default to the new one in v5. Does that make sense? I just want to make sure we don't take on the burden of questions and support if it is not necessary to remove.

[benmccann]

The only thing I could think of is loading in the extra lib, but frankly unless it can be proven to make a meaningful difference in startup time I am inclined to take that penalty so that we avoid increased issue volume on the project.

A couple other concerns related to having extra dependencies might be increased supply chain risk and increased risk of deprecation warnings. The latter has been a headache for me in some projects I maintain where a library gets deprecated and then there's difficulty in upgrading for a number of reasons - e.g. the latest version is deprecated and we need to move to a new library, the latest in a minor is deprecated so we need to move to a new major but it introduces breaking changes, the deprecation is down the dependency tree and there's a dependency in between that's unmaintained or otherwise won't update.

I just want to make sure we don't take on the burden of questions and support if it is not necessary to remove.

I'm sure you're aware, but there's also a burden of having to support multiple options. It's more code paths, is harder to get bug reporters to clearly communicate which option they're using, etc. I also think it's harder as a user to have multiple ways to do things as it means you have to read more documentation to understand how things work, might find tutorials or stackoverflow answers talking about the other way, etc.

[wesleytodd]

A couple other concerns related to having extra dependencies might be increased supply chain risk and increased risk of deprecation warnings

But this one is a node built in. I do no think this is applicable in this case.

I'm sure you're aware, but there's also a burden of having to support multiple options.

I am, and usually I would oppose an addition adding an alternate with duplicate functionality, but in this case it is adding support for a web standard where none existed when the original was written so to me that is a good thing. The thing I am worried about is that we missed something required by come applications and have to revisit this decision after releasing. I would rather take the low cost supporting two options than have to re-litigate this discussion in 3 months when we release v5 and find out URLSearchParams had some difference which matters to some users.

I also think it's harder as a user to have multiple ways to do things as it means you have to read more documentation to understand how things work, might find tutorials or stackoverflow answers talking about the other way, etc.

Totally agree here, but with so few folks who are regularly showing up to support and answer users questions and issues we have to make some compromises. In this case I believe that the risk of switching without a fallback to the ~10y old behavior is high enough to warrant the decision, but I am happy to hear differently if you (or anyone else) strongly believe this is best and bring the backing argument for it.