Acceptance of changes made to the legacy APIs

[RaisinTen]

Should we accept changes made to the legacy APIs? If so, what kinds of changes should we accept?

For reference, this came up when we were discussing about whether we should accept a change made to the legacy url.parse() API in #42196.

cc @nodejs/tsc

[benjamingr]

I think "legacy" means "the project does not intend to strategically spend a lot of time adding features to this". If people want to contribute improvements to usability or in general as long as it doesn't make maintenance harder I think that's fair game?

[RaisinTen]

If the project accepts changes that improve the usability of a legacy API, it conflicts with what the project currently states -

• node/doc/api/documentation.md

  Lines 43 to 45 in 3d4f3ca

  	> Stability: 3 - Legacy. The feature is no longer recommended for use. While it
  	> likely will not be removed, and is still covered by semantic-versioning
  	> guarantees, use of the feature should be avoided.

• node/doc/api/url.md

  Lines 1562 to 1566 in 3d4f3ca

  	Use of the legacy `url.parse()` method is discouraged. Users should
  	use the WHATWG `URL` API. Because the `url.parse()` method uses a
  	lenient, non-standard algorithm for parsing URL strings, security
  	issues can be introduced. Specifically, issues with [host name spoofing][] and
  	incorrect handling of usernames and passwords have been identified.

Reading these I get the feeling that the project explicitly discourages uses of these APIs.

[benjamingr]

Reading these I get the feeling that the project explicitly discourages uses of these APIs.

Right - why do you think that conflicts? We can recommend one API (WHATWG URL) while supporting two (WHATWG URL and url) and still improving both though strategically only investing in one can't we?

[mcollina]

Usually legacy APIs are so because we can't improve them without breaking the ecosystem. This would rule out most semver-major changes, however bugfixes and semver-minor could be accepted.

[RaisinTen]

Right - why do you think that conflicts?

I think they conflict because on the one hand we want folks to migrate to the better API while on the other, we are still giving folks reason to stay on the broken API.

We can recommend one API (WHATWG URL) while supporting two (WHATWG URL and url) and still improving both though strategically only investing in one can't we?

I believe that would have been okay if the document said "not actively supported" instead of "discouraged".

Usually legacy APIs are so because we can't improve them without breaking the ecosystem. This would rule out most semver-major changes, however bugfixes and semver-minor could be accepted.

But isn't that true for stable APIs too? What's the point of labelling the API as legacy if we still treat it as a stable API? Also, instead of applying fixes / semver-minor changes to an API that is inherently broken and unsafe to use, won't it be easier for folks to just move to the recommended API?

[benjamingr]

But isn't that true for stable APIs too? What's the point of labelling the API as legacy if we still treat it as a stable API?

Basically, I've always read it as an indication that this API should not be adopted in new project since Node is not going to strategically invest a lot of time on it.

If there is an API we'll never remove but want people to really not use it's usually "forever deprecated" like new Buffer and friends.

[targos]

If there is an API we'll never remove but want people to really not use it's usually "forever deprecated" like new Buffer and friends.

I thought this was the point the Legacy status. Basically a forever doc-only deprecation.

[RaisinTen]

If there is an API we'll never remove but want people to really not use it's usually "forever deprecated" like new Buffer and friends.

node/doc/api/documentation.md

Lines 26 to 27 in 3d4f3ca

	> Stability: 0 - Deprecated. The feature may emit warnings. Backward
	> compatibility is not guaranteed.

doesn't say that the deprecated APIs will never be removed. Only the legacy status says that. In fact, the third stage of our deprecation policy, "End-of-Life", explicitly states that the API is/will be removed -

node/doc/api/deprecations.md

Lines 33 to 34 in 3d4f3ca

	An End-of-Life deprecation is used when functionality is or will soon be removed
	from Node.js.

.

I thought this was the point the Legacy status. Basically a forever doc-only deprecation.

Documentation-only deprecation is different -

node/doc/api/deprecations.md

Lines 19 to 26 in 3d4f3ca

	A Documentation-only deprecation is one that is expressed only within the
	Node.js API docs. These generate no side-effects while running Node.js.
	Some Documentation-only deprecations trigger a runtime warning when launched
	with [`--pending-deprecation`][] flag (or its alternative,
	`NODE_PENDING_DEPRECATION=1` environment variable), similarly to Runtime
	deprecations below. Documentation-only deprecations that support that flag
	are explicitly labeled as such in the
	[list of Deprecated APIs](#list-of-deprecated-apis).

. It only applies for APIs we really want to deprecate, which is assigned a stability of 0, unlike legacy status, which is assigned a stability of 3.

[mcollina]

But isn't that true for stable APIs too? What's the point of labelling the API as legacy if we still treat it as a stable API? Also, instead of applying fixes / semver-minor changes to an API that is inherently broken and unsafe to use, won't it be easier for folks to just move to the recommended API?

Unfortunately that's not easy at all. Usually those legacy APIs are used in very old modules that have all the quirks figured out. Changing it in 3-4-5 level deep is very hard.

[RaisinTen]

To make it easier for users, would it be reasonable to write a guide like we did in https://nodejs.org/en/docs/guides/buffer-constructor-deprecation/?

Also, are you trying to say that it is easier to make the needed changes to the parser in Node.js than to the code that uses the legacy API inside the older module? If that's so, I was wondering if the migration strategy I stated in #42196 (comment) sounds practical:

"We'll gradually make changes to url.parse() to make it feel more like WHATWG URL. Since the differences between the two includes many breaking changes, we'll land those too, so that it's easier for users to adapt to WHATWG URL bit by bit than to ask them to make changes to their code to be WHATWG URL-friendly all at once."

Yes, it does talk about making semver-major changes but if we do them gradually, it should be helpful for users to migrate. Not sure why we are ruling out semver-major changes for legacy APIs.

[Trott]

What's the point of labelling the API as legacy if we still treat it as a stable API?

The stability labels aren't about differences in our process, although that can be part of it. But the labels are for users, not us. The label says "Don't use this in new code." It does not require us to have a different maintenance process.

[Trott]

Should we accept changes made to the legacy APIs?

Yes.

If so, what kinds of changes should we accept?

Changes that improve the API without causing widespread breakage for users and that do not substantially increase our maintenance burden.

[RaisinTen]

The stability labels aren't about differences in our process, although that can be part of it. But the labels are for users, not us.

I thought these labels were supposed to convey to the users how we maintain these APIs, so it is a result of us having different maintenance processes for each.

The label says "Don't use this in new code."

It doesn't specifically say "new code", so it stands for both new and old code.

[Trott]

Should we accept changes made to the legacy APIs? If so, what kinds of changes should we accept?

I think it's useful to ask the question like this:

Should we make an effort to intentionally preserve security vulnerabilities and other bugs in Legacy APIs so that it inflicts pain on end users when that user is using the API, even if it is in a transitive dependency?

^{EDIT: Just to make it 100% clear that I'm NOT advocating the pro-bugs pro-user-pain perspective, I'll mention that I definitely think we should allow fixes. I mean, obviously. But just in case it wasn't obvious.}

[jasnell]

Bug fixes that don't break backwards compat or introduce significant regressions: Yes if there are people who want to work on it.

New features, breaking changes: No.

In the case of something like url.parse, I would view making it act more like the WHATWG parser as falling under the New Feature category. That is, url.parse was never meant to be compliant with WHATWG so changing it to start being compliant qualifies as new function and new feature. Making it do so just signals that folks are fine to keep using the API in new things which is counter to the purpose of marking it as Legacy.

[RaisinTen]

Then what qualifies as a bug in the context of url.parse()? Did we follow a spec while implementing it? If so, what spec did we follow?

[cjihrig]

My opinion is that we should treat Legacy APIs the same as all other Stable APIs as long as people are volunteering to do the work, and the Legacy designation should only be used to discourage their use in new code.

[jasnell]

With regards to things like url.parse, I certainly would not block any prs to update it unless those are semver-major. I would just strongly discourage such changes. If folks want to invest time in an old, node specific, non standard, non portable, non spec compliant API when there is a standardized, cross platform, portable, spec compliant alternative available, that's up to them, but please don't make breaking changes.

[Trott]

Then what qualifies as a bug in the context of url.parse()?

@RaisinTen The same things that qualify as bugs in every other context.

I would view making it act more like the WHATWG parser as falling under the New Feature category

@jasnell I would agree if it's "make it behave like WHATWG parser for the sake of making it behave like WHATWG parser", then we shouldn't do that. I'm OK with (and it seems that you are at least tolerant of) "hey, there's a bug or security issue, and one way to fix it is to emulate WHATWG parser in this narrow edge case." An example of that is #42196.

I also agree that we don't have to encourage them and we can be very cautious about accepting such changes. But obviously, that's not the same thing as automatically rejecting such fixes and insisting on leaving potentially-risky edge case bugs in place.

[ljharb]

It shouldn’t take a stick to get people to migrate. If the incremental bug fixes make the old api “good enough that i don’t need the new api” (such that without the fixes, it wouldn’t meet that bar) then the new api clearly failed at being better enough to be worth it.

The only way to prove a new thing is better is to make the old thing as good as possible, and for people to still prefer that new thing.

Is URL that underwhelming that these minor bugfixes are going to retroactively make the entire effort a waste of time? I certainly don’t think so, but blocking these bugfixes imo is representing exactly that opinion.

[jasnell]

To be clear, no one has suggested in any way that bug fixes should be blocked.

[ljharb]

Isn’t this very issue asking if bug fixes to legacy APIs should be blocked or not? If not, then I’m not clear on what’s being discussed, and the linked PR can land prior to resolving this issue - but there’s a block on it.

[jasnell]

That's what the issue is asking, yes, but as far as I'm aware no one has suggested that the answer is to block bug fixes.

[ljharb]

Great! So is there any reason not to make the policy be “accept non-breaking bugfixes made to legacy APIs, assuming they don’t increase maintenance burden or unreasonably harm performance (or other similar ad hoc caveats)”?

[fhinkel]

We don't block bug fixes on legacy APIs.

[jasnell]

What I think does merit conversation, however, is what qualifies as a bug fix vs new feature. Updating a legacy API to conform to a standard that didn't exist when the API was added, and was never claimed to conform to doesn't qualify as a bug fix to me, especially when there's a fully supported non-legacy API that already does conform to that spec available. The message you're essentially sending is that there's no reason to migrate to the new API since we're eventually going to make this old one act the same.

[Trott]

Updating a legacy API to conform to a standard that didn't exist when the API was added, and was never claimed to conform to doesn't qualify as a bug fix to me, especially when there's a fully supported non-legacy API that already does conform to that spec available.

I would agree with that. As I wrote above: "I would agree if it's 'make it behave like WHATWG parser for the sake of making it behave like WHATWG parser', then we shouldn't do that. "

[RaisinTen]

What I think does merit conversation, however, is what qualifies as a bug fix vs new feature. Updating a legacy API to conform to a standard that didn't exist when the API was added, and was never claimed to conform to doesn't qualify as a bug fix to me, especially when there's a fully supported non-legacy API that already does conform to that spec available.

The problem here is that folks can end up a building a "new feature" in the process of fixing a "bug" and vice versa, so this sounds ambiguous to me.

I would agree if it's 'make it behave like WHATWG parser for the sake of making it behave like WHATWG parser', then we shouldn't do that.

Reading the current description in #42196:

Emulate the WHATWHG URL parse behavior of trimming leading and trailing
C0 control characters. This moves url.parse() slightly closer to
WHATWHG URL behavior. The current behavior is possibly insecure for some
uses.

I understand that the PR tries to conform the legacy API to a standard which didn't exist when the API was added. And in doing so, it ends up fixing a bug. This feels like a "new feature" which would be blocked according to #42230 (comment) (New features, breaking changes: No.).

However, the ordering of sentences in the description could easily be flipped:

The current behavior is possibly insecure for some uses. Emulate the
WHATWHG URL parse behavior of trimming leading and trailing C0 control
characters. This moves url.parse() slightly closer to WHATWHG URL
behavior.

and now I understand that there is a bug in a legacy API and this PR tries to fix it by adopting the behavior of a standard because it is more reliable. This sounds like a "bug fix" to me which should be approved.

I think it would be clearer if we considered all semver-patch commits as bug fixes, even if the patch tries to conform the legacy API to a standard that might not have existed during its creation.

---

The message you're essentially sending is that there's no reason to migrate to the new API since we're eventually going to make this old one act the same.

The problem is that both the legacy API and the newer API has some advantages and disadvantages and none of them are good enough to rule out each other in the current state. This is the reason why we aren't supportive of even runtime deprecating the legacy APIs.

[RaisinTen]

Also, if we don't intend to go ahead with runtime deprecating / End-of-Life deprecating an API, I don't think we should "discourage" folks from using it. "Not encouraging" is okay IMO.

[ljharb]

If the new one isn’t good enough to rule out the old one, then I’d hope that correcting that would be a priority for those who want(ed) the new api, and also for the old one to be gone.

[jasnell]

If the new one isn’t good enough to rule out the old one, then I’d hope that correcting that would be a priority for those who want(ed) the new api, and also for the old one to be gone.

This is a bit silly. Unless the old API is removed entirely, there will always be folks who insist on using the old API no matter what improvements are made to the new API simply because they don't want to change or changes would be too disruptive in other ways. None of this has anything to do with whether the new API is "good enough". For instance, the reason url.parse() was not deprecated is that there was no reason to force people off of it despite the very real bugs and lack of a standard API. The presence of a better new API doesn't mean that folks should have to migrate off the old. That has nothing to do with quality of the new API and framing it in that way is not helpful at all.

I don't think we should "discourage" folks from using it

Unless there are very real reasons folks shouldn't be using it and should be using the new one, which in the case of url.parse() there are. But again, those reasons aren't enough to force folks off url.parse() so just keeping it as it is should be fine.

I think it would be clearer if we considered all semver-patch commits as bug fixes, even if the patch tries to conform the legacy API to a standard that might not have existed during its creation.

I don't believe anyone has argued against this even if I don't always agree such changes are necessary (note, for instance, that I haven't blocked any of these type). semver-minor should be given a very critical eye and semver-major should be avoided entirely unless it is considered a critical security fix.

If folks want to keep maintaining the legacy API, more power to them! Great to have contributions in all cases. But maintaining two APIs that do generally the same thing increases the maintenance burden and cognitive load for everyone -- maintainers and users -- so it's definitely worth keeping that in mind.

[ljharb]

@jasnell to clarify, i'm not claiming that all desire for the old one must be eliminated - i'm saying that assuming the claim "The problem is that both the legacy API and the newer API has some advantages and disadvantages and none of them are good enough to rule out each other in the current state" is true, then that suggests there's an opportunity for improvement in the new API. I'm not making any assertions about the claim being true, however.

[Trott]

The problem here is that folks can end up a building a "new feature" in the process of fixing a "bug" and vice versa, so this sounds ambiguous to me.

I'm OK with the ambiguity. I don't think this is a problem that we need to solve in advance. It might never actually come up. And if it does, we can weigh at that time what to do based on the pros and cons of the particular situation. Trying to set policy in advance for this seems like it may be an anti-pattern to me. It's trying to make a decision on something hypothetical. If we wait until it becomes real, we'll have actual information to weigh. There will be less guesswork. Additionally, it might never come to pass at all.

I could be wrong, but I think we're going down a bit of a rabbit-hole at this point and we can close this issue. I'm fine with leaving it open longer if others disagree. But it seems to me that there is consensus on:

• Fixing bugs in Legacy APIs is absolutely permitted.
• New features would be more controversial and would need a very compelling case. "This bug is a potential security issue and can't be effectively fixed without a new feature" would be compelling to me. "This will make the Legacy API easier for folks to use" is less compelling, although it could be compelling if the use case is one that the new API doesn't or can't handle. All this probably needs to be evaluated on a case-by-case basis. Fortunately, these cases seem to be rare.
• Breaking changes for url.parse() specifically are a problem. Not sure about other Legacy APIs, but it probably applies there too. Then again, I don't think we need a one-size-fits-all policy necessarily.

And going forward, for Legacy API changes, I'll be sure to emphasize the bug-fix nature of any bug-fix changes, since that at least seems to be an area of especially broad agreement.

[RaisinTen]

I don't think we should "discourage" folks from using it

Unless there are very real reasons folks shouldn't be using it and should be using the new one, which in the case of url.parse() there are.

Won't that be applicable the other way round too? The points in #42232 (comment) are some reasons to discourage the uses of the newer API and use the legacy API instead. I don't think we should discourage/encourage the usage of any of these. In stead, we can make it very clear in the docs that the project does not officially support one of the APIs and then let the users decide if they want to use it or not.

And if we choose to discourage uses of the legacy API (which we should talk about in #42232), I think we should do it through a runtime warning because it communicates the message better. We don't have to go ahead with the End-of-Life deprecation though (which is what's happening with the Buffer c'tor).

But again, those reasons aren't enough to force folks off url.parse()

Then saying "uses in newer code is discouraged" sounds more fitting?

[RaisinTen]

PR: #42269

[aduh95]

Maybe we could get the habit of running CITGM for any changes to a legacy API, and withdraw the PR if that makes any package fail. Not sure if it needs to be a documented process, but that could help having revert PRs (e.g. #42280).

[RaisinTen]

Legacy APIs in particular or all stable APIs (which includes legacy APIs because they are maintained in the same way) in general?

[ljharb]

Seems like something we’d want to do on any PR.

[targos]

It's something we'd want to do if it didn't take many hours to run and many minutes, if not hours, to analyze the results.