Remove legacy status from querystring

[mcollina]

The node:querystring module is significantly faster than URLSearchParams (ref. https://github.com/anonrig/fast-querystring). Given that querystring is marked as legacy, quite a few people are continuosly opening issues on some modules I maintain asking to replace querystring with URLSearchParams... however, we really can't as there is a significant performance drop.

Therefore... can we remove the legacy status?

[mcollina]

cc @nodejs/tsc

[RaisinTen]

Given that querystring is marked as legacy, quite a few people are continuosly opening issues on some modules I maintain asking to replace querystring with URLSearchParams

Why? Only new code should use URLSearchParams (our docs already say that) -

node/doc/api/querystring.md

Lines 18 to 19 in 6475a87

	The `querystring` API is considered Legacy. While it is still maintained,
	new code should use the {URLSearchParams} API instead.

. It's fine if existing code keeps using the legacy APIs.

[mcollina]

I do not know why it is a problem for folks, but it happens a few times a month and is quite annoying.

[RaisinTen]

I wonder if it's more of a visibility issue. Maybe we should highlight this statement in the docs or rephrase it to better express that it's okay to use legacy APIs?

[Trott]

I do not know why it is a problem for folks, but it happens a few times a month and is quite annoying.

@mcollina Can I open PRs on your projects adding code comments explaining that querystring is being used because it is more performant than URLSearchParams and to please not open PRs trying to change it unless benchmarks can show equivalent or better performance? 😀

I wonder if it's more of a visibility issue. Maybe we should highlight this statement in the docs or rephrase it to better express that it's okay to use legacy APIs?

@RaisinTen I'm not opposed to trying that, but my inclination is to remove the Legacy status but leave a statement explaining that people should consider using URLSearchParams instead if they want to use something identical to browser APIs (or whatever the reason is--clearly performance is not one of them).

I've never liked the Legacy status, although I understand why it was created.

[mcollina]

This is just a few days ago: nodejs/undici#1682.
Then fastify/fastify#4239, fastify/fastify#3645.

Possibly a few more.

My take is that we are not doing a good service to the community: if I didn't know URLSearchParams was significantly slower then querystring, I would have caved in. There is no good reason to switch.

(This is not the same of new URL(), which is slower than url.parse() but it's correct and secure).

(Note: TypeScript signals all legacy marked APIs as @deprecated.

[Trott]

Note: TypeScript signals all legacy marked APIs as @deprecated.

Another reason to get rid of Legacy entirely. The distinction between Legacy and Deprecated has never been clear. (Supposedly, Legacy is "we don't plan to remove it ever" but that's also true of Deprecated APIs like sys and domain. So the existence of Legacy is more confusing than enlightening in my opinion.)

[gireeshpunathil]

from the linked issues, it is evident that folks misunderstand legacy to be same as deprecated, or moving towards deprecated.

Potential options are:

• disambiguate legacy and deprecated
• remove legacy entirely, and make the incumbent APIs some sub-type of deprecated

[Trott]

#44912

[Trott]

Potential options are:

• disambiguate legacy and deprecated
• remove legacy entirely, and make the incumbent APIs some sub-type of deprecated

I think, when possible, it is better to have fewer categories that are intuitively understood. In that regard, I think we should remove Legacy. Some of the Legacy APIs (notably url.parse()) can be doc-deprecated. Others can be returned to Stable with a note explaining when to use something else instead. (Thats' the approach I took in #44912.)

[mcollina]

I'd be ok in doc-deprecating url.parse().

[GeoffreyBooth]

I wonder how much of this is TypeScript’s treating “legacy” as deprecated. We could reach out to the @types maintainers to ask them to revise that.

Ultimately we do need a status that means “not getting removed, but closed to further development.” Most of CommonJS falls under that category. If not “legacy,” perhaps “frozen”?

[Trott]

Ultimately we do need a status that means “not getting removed, but closed to further development.”

I mean, the proper term for that is "deprecated". It's unfortunate, though, that "deprecated" for most people means "will be removed". I don't know how/when that shift happened, but it's unfortunate, because "deprecated" is a useful term made less useful that way.

We could go with something very explicit: "Usage discouraged." But that's literally the English language definition of "deprecated" so I'm not sure that won't result in more confusion.

Ultimately, I question whether we really need a term for "not getting removed, but closed to further development".

[Trott]

Ultimately, I question whether we really need a term for "not getting removed, but closed to further development".

To elaborate: No matter what we chose, whether it's Legacy or Frozen or Stop-Using-This-In-New-Code-But-Leave-It-Alone-In-Old-Code-It's-Fine-Don't-Ask-Us-Why-Because-We're-Not-Sure-Either, people will infer that they need to update code.

The correct label for "you can leave this alone and use it, it's fine" is Stable and we should use that.

[GeoffreyBooth]

The correct label for “you can leave this alone and use it, it’s fine” is Stable and we should use that.

The other idea I considered was splitting Stable into “Stable: Development Continuing” and “Stable: Development Frozen”. But that’s unwieldy. Though it perhaps clarifies that really these are two attributes that we should stop trying to conflate into one. A Node API really has two statuses, for two demographics:

• For end users: should they use it?
• For Node.js contributors: should they continue to improve it?

The idea of “legacy” is really aimed for the latter group: it tells contributors to stop considering new enhancements to that API. It’s meaningless to end users, who really should consider the API to be stable. But if we call it stable, then we should probably have some other way to signal to contributors that the API is frozen.

[anonrig]

I'm in favor of removing legacy and improving querystring module performance.

[kibertoad]

We could go with something very explicit: "Usage discouraged." But that's literally the English language definition of "deprecated" so I'm not sure that won't result in more confusion.

But that's the actually important part of the whole thing. It should not be discouraged. It's faster. It's stable. There is nothing wrong with it.

[Trott]

We could go with something very explicit: "Usage discouraged." But that's literally the English language definition of "deprecated" so I'm not sure that won't result in more confusion.

But that's the actually important part of the whole thing. It should not be discouraged. It's faster. It's stable. There is nothing wrong with it.

In this case, yes. But that conversation (which is getting off-topic admittedly) was about APIs that we do want to discourage. (I think "deprecated" is preferable for those over having some things "deprecated" and others "discouraged". But again, that's veering off-topic here.)

[cysp]

Out of interest, is there a specification-related reason that explains why URLSearchParams' stringification is more expensive than querystring's or is it "simply" an implementation issue?

Looking at the referenced benchmarks, I noticed that the URLSearchParams parsing benchmark code does more than just parsing, it also dynamically constructs objects with the parsed results. That might be a valid benchmark comparison in some contexts where the required result is a plain javascript object, but if the user of the parser wants to access parameters off the parsed object directly none of that would be necessary. Removing those object creation operations gives a benchmark result that shows URLSearchParams' parsing is actually more efficient than the alternatives (at least on my M1 machine), using node v16 and v18.
(This might be known to those already in this conversation but I haven't seen it mentioned anywhere.)

Benchmark results:

> node benchmark/parse.mjs

╔═════════════════════════════════════════╤═════════╤═══════════════════╤═══════════╗
║ Slower tests                            │ Samples │            Result │ Tolerance ║
╟─────────────────────────────────────────┼─────────┼───────────────────┼───────────╢
║ qs                                      │   10000 │  293184.20 op/sec │  ± 1.49 % ║
║ query-string                            │   10000 │  337745.45 op/sec │  ± 1.07 % ║
║ querystringify                          │   10000 │  407290.77 op/sec │  ± 2.19 % ║
║ @aws-sdk/querystring-parser             │   10000 │  454130.73 op/sec │  ± 1.39 % ║
║ URLSearchParams-with-Object.fromEntries │   10000 │  736588.35 op/sec │  ± 3.83 % ║
║ URLSearchParams-with-construct          │   10000 │ 1169521.29 op/sec │  ± 2.02 % ║
║ node:querystring                        │   10000 │ 1474337.17 op/sec │  ± 4.36 % ║
║ querystringparser                       │   10000 │ 1900163.87 op/sec │  ± 3.88 % ║
║ fast-querystring                        │   10000 │ 1992138.62 op/sec │  ± 4.10 % ║
╟─────────────────────────────────────────┼─────────┼───────────────────┼───────────╢
║ Fastest test                            │ Samples │            Result │ Tolerance ║
╟─────────────────────────────────────────┼─────────┼───────────────────┼───────────╢
║ URLSearchParams                         │    4000 │ 2371200.82 op/sec │  ± 0.99 % ║
╚═════════════════════════════════════════╧═════════╧═══════════════════╧═══════════╝

Stringifying a URLSearchParams object, however, is definitely more expensive.

[jasnell]

@cysp ... I think it's more that it's never been performance optimized at all. I wrote it to implement correct behavior, not to be fast. There's likely a lot of room for improvement.

[anonrig]

@cysp I'm the author behind fast-querystring. You are partially correct. The performance issue in fast-querystring and all other javascript based querystring parsers and stringifiers are indeed related to the performance bottleneck of JavaScript. But you should know that in most of the use cases, the developer does not call params.get(key) but directly passes the query_params as an object to a validator (which consumes it as object).

On the other hand, I agree with @jasnell. URLSearchParams uses toUSVString which has a performance bottleneck due to JS/C++ communication.