Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: clarify the meaning of legacy status #42269

Merged
merged 11 commits into from
Mar 14, 2022
Merged

doc: clarify the meaning of legacy status #42269

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3323b8a
doc: clarify the meaning of legacy status
RaisinTen Mar 9, 2022
128c438
fixup! Update doc/api/documentation.md
RaisinTen Mar 9, 2022
75c4766
mixup! Update doc/api/url.md
RaisinTen Mar 9, 2022
d437706
fixup! mixup! Update doc/api/url.md
RaisinTen Mar 9, 2022
471d162
fixup! fixup! mixup! Update doc/api/url.md
RaisinTen Mar 9, 2022
e3d9b27
fixup! Update doc/api/deprecations.md
RaisinTen Mar 9, 2022
4273271
doc: clarify the meaning of legacy status
RaisinTen Mar 12, 2022
0334b1c
fixup! Update doc/api/documentation.md
RaisinTen Mar 12, 2022
85ace39
doc: use lowercase for Legacy and Deprecated
RaisinTen Mar 13, 2022
71fa00c
Update doc/api/url.md
RaisinTen Mar 13, 2022
cc7d2dd
Update doc/api/url.md
RaisinTen Mar 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

<!-- type=misc -->

Node.js APIs might be deprecated for any of the following reasons:
Node.js APIs might be deprecated or given legacy status for any of the
following reasons:

* Use of the API is unsafe.
* An improved alternative API is available.
Expand Down
2 changes: 1 addition & 1 deletion doc/api/documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ The stability indices are as follows:

> 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.
> guarantees, use of the feature should be avoided if possible.
Copy link
Contributor

@bnb bnb Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep the stronger wording. In my experience people will ignore stronger warnings when it's not possible, but will pick up something that's got a softer warning effectively because of logic along the lines of "how bad could it be".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even strengthen it further by making it active voice. (Suggestion to follow....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too would prefer the stronger wording if we were okay with emitting a runtime warning (this is not the same as removing the API), which people seem to be -1 about.

Copy link
Member

@Trott Trott Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking suggestion if we want to warn people away from Legacy APIs a bit more vigorously:

Suggested change
> 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.
> guarantees, use of the feature should be avoided if possible.
> Stability: 3 - Legacy. Avoid using the feature. Although it is unlikely
> to be removed and is still covered by semantic-versioning guarantees,
> there are more robust and secure alternatives.

I'm also fine with the if possible being left in place or removed. These are all wordsmithing things that can be iterated on later. Any of them will work for now as far as I'm concerned.

Copy link
Contributor

@isaacs isaacs Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to add something to the effect of "it is not necessary to track down all instances of this feature in every module in your dep tree and spam the maintainers with PRs" but maybe with like 80% less snark than that 😅

> Stability 3 - Legacy. Avoid using the feature in new code. Although it is
> unlikely to be removed and is still covered by semantic-versioning
> guarantees, it is no longer actively maintained, and better options are
> available. Features are marked as Legacy rather than being Deprecated if
> their use does no harm, and they are widely relied upon within the npm
> ecosystem. Bugs found in Legacy features are unlikely to be fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that can go directly below rather than inside the banner?

Suggested change
> 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.
> guarantees, use of the feature should be avoided if possible.
> Stability: 3 - Legacy. Avoid using the feature. Although it is unlikely
> to be removed and is still covered by semantic-versioning guarantees,
> there are more robust and secure alternatives.
Features are marked as Legacy rather than Deprecated if they are too
widely relied upon within the npm ecosystem to be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "if their use does no harm" part is pretty useful; it tells everyone that there is nothing wrong with using Legacy things, and thus no reason to pester maintainers to migrate away from them.

Copy link
Member

@Trott Trott Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "if their use does no harm" part is pretty useful;

I'm not sure everyone would agree with the characterization that Legacy APIs do no harm and that there's nothing wrong with using them. Regarding url.parse(), our docs currently say this:

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.

I would consider that at odds with "there is nothing wrong with using" it, although I'm sure there are semantic arguments to be had here.

Copy link
Contributor Author

@RaisinTen RaisinTen Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure everyone would agree with the characterization that Legacy APIs do no harm and that there's nothing wrong with using them.

If the API is actually harmful and there is no way to fix such issues, I think we should help our users migrate to the recommended API by emitting a runtime warning and deprecate it because we don't want our users to get harmed.

I think https://stackoverflow.com/a/2873695 sums up what deprecated and legacy means really well:

Deprecation means that it's bad and shouldn't be used.

Legacy just means that it's old and there are ways of doing something which are generally, but not necessarily, better.

Given that url.parse() has a performance benefit and also makes it easier to handle relative URLs, I don't see why people should not use it even in newer code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly, PTAL.


Use caution when making use of Experimental features, particularly within
modules. Users may not be aware that experimental features are being used.
Expand Down
9 changes: 4 additions & 5 deletions doc/api/url.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1558,11 +1558,10 @@ A `TypeError` is thrown if `urlString` is not a string.

A `URIError` is thrown if the `auth` property is present but cannot be decoded.

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.
Avoid `url.parse()` if you can. Use the WHATWG `URL` API instead.
`url.parse()` uses a lenient, non-standard algorithm for parsing URL
strings. It is prone to security issues such as [host name spoofing][]
and incorrect handling of usernames and passwords.

### `url.resolve(from, to)`

Expand Down