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.

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

Are we ready to deprecate url.parse() now? #42232

Closed
2 of 3 tasks
RaisinTen opened this issue Mar 6, 2022 · 7 comments · Fixed by #42269
Closed
2 of 3 tasks

Are we ready to deprecate url.parse() now? #42232

RaisinTen opened this issue Mar 6, 2022 · 7 comments · Fixed by #42269
Labels
deprecations Issues and PRs related to deprecations. url Issues and PRs related to the legacy built-in url module.

Comments

@RaisinTen
Copy link
Contributor

Node.js APIs might be deprecated for any of the following reasons:
* Use of the API is unsafe.
* An improved alternative API is available.
* Breaking changes to the API are expected in a future major release.

  • Use of the API is unsafe. - Yes.
  • An improved alternative API is available. - My understanding is that the WHATWG URL API is supposed to be the improved alt API, so I'll leave it checked for now unless anyone has any objections.
  • Breaking changes to the API are expected in a future major release. - I guess that depends on whether we will proceed with the deprecation, so I guess it's undecided for now?

Since at least one of the points above holds true, it should qualify for a deprecation, but hey, the deprecation status was previously revoked and it was moved to legacy status in #37784, so I'm doubtful that it would be that straightforward. I don't know why the deprecation was revoked but it would be nice to know the reason and if it still applies, we should update the doc accordingly.

cc @nodejs/tsc

@targos
Copy link
Member

targos commented Mar 6, 2022

As I said somewhere else, to me, Legacy status is basically the same as doc-deprecated, except it will never be runtime-deprecated or removed. So I wouldn't say that the deprecation was revoked.

@mcollina
Copy link
Member

mcollina commented Mar 6, 2022

I'm -1, legacy is ok.

A few unrelated example:

  1. url.parse() is faster than new URL(). querystring is also significantly faster.
  2. url.parse supports a few use cases that are not supported with new URL().
  3. it was widely used and we'd need to gather some feedback that's not the case but I doubt it.

@Trott
Copy link
Member

Trott commented Mar 6, 2022

While I'd be OK (and would even prefer) doc-deprecating things forever rather than having a separate Legacy status, I'm also OK if others prefer continuing to use Legacy as "doc-deprecated forever" instead. And since it appears others would indeed prefer to continue to use Legacy as "doc-deprecated forever", then I say let's just do that.

If the intention of doc-deprecating is to signal that we plan to eventually runtime deprecate and remove, then that is an indication that, unfortunately, "Legacy" is still a useful concept because people continue to get the wrong idea (in my opinion) about what a deprecation is. A deprecation is not (in my opinion) a commitment to remove something. It is simply an indication that something is obsolete and/or not recommended. But since people have insisted in the past that it somehow makes no sense to deprecate something forever without removing it, then I guess here we are. Legacy it is.

@VoltrexKeyva VoltrexKeyva added url Issues and PRs related to the legacy built-in url module. deprecations Issues and PRs related to deprecations. labels Mar 7, 2022
@RaisinTen
Copy link
Contributor Author

As I said somewhere else, to me, Legacy status is basically the same as doc-deprecated, except it will never be runtime-deprecated or removed. So I wouldn't say that the deprecation was revoked.

Should we also clarify in the docs that legacy falls into the deprecation category? What should we write in

Type: Deprecation revoked
?

A deprecation is not (in my opinion) a commitment to remove something. It is simply an indication that something is obsolete and/or not recommended.

That's what legacy / doc-deprecated means. I think runtime deprecated should mean that there is a chance (not a guarantee) that we will remove the API in the future, otherwise the end-of-life deprecation (which currently means that a feature is or will be removed and I believe it should mean that the feature has been removed) comes out of nowhere.

RaisinTen added a commit to RaisinTen/node that referenced this issue Mar 9, 2022
@RaisinTen
Copy link
Contributor Author

PR: #42269

@isaacs
Copy link
Contributor

isaacs commented Mar 11, 2022

This is require('sys') all over again.

I'm with @Trott on this. "Deprecation" means, colloquially, that something may be removed in the future, but definitely that it is no longer officially supported or recommended. For a platform, there is no reason to remove a deprecated feature unless it is actively causing harm (either because it is unsafe to use at all, or because its continued existence is costly). Ie, "if you find a bug in this, tough luck, but if it works for you, more power to you."

In Node, this has come to mean "deprecated prints a run-time warning", which is just obnoxious. This is much more forceful than just "not recommended", it's actually annoying to users, it's an obstacle to upgrading, and causes friction in our ecosystem. Anything deprecated in this way should be removed in the next major version or 2, because (a) the warning is annoying, remove it, and so (b) if it was bad enough to justify a warning, it's bad enough to justify removal.

Is url.parse() actively harmful? No. It's plain old string-parsing JavaScript with no dependencies. It doesn't stand in the way of any other development, and since it's doc-deprecated, it's zero maintenance cost. Adding a run-time deprecation would be rude. All it would do is make a lot of people get warnings when they upgrade Node, even though everything else works fine, and they'd have no easy way to fix it except nagging maintainers to update modules they haven't had to touch in years. It would be an incentive to just turn off warnings, or worse, stop caring about them.

And as @mcollina points out, it can do things that new URL can't, and is faster in many use cases. No only is it not harmful, it's actually a slight benefit to have it. Idk if it would be worth adding today if it wasn't there, all things considered, but leaving it there costs little (especially compared with removing it) and adds some actual value.

It's weird to have "legacy" and "deprecated" mean subtly different things, I agree, but in this case, I think it's valuable, and #42269 is the right direction to take.

nodejs-github-bot pushed a commit that referenced this issue Mar 14, 2022
Fixes: #42230
Fixes: #42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
bengl pushed a commit that referenced this issue Mar 21, 2022
Fixes: #42230
Fixes: #42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fixes: nodejs#42230
Fixes: nodejs#42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: nodejs#42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #42230
Fixes: #42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #42230
Fixes: #42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #42230
Fixes: #42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
Fixes: nodejs#42230
Fixes: nodejs#42232

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: nodejs#42269
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@sneakyfildy
Copy link

what was the reason to deprecate faster more convenient method?

something-predictable pushed a commit to something-predictable/node-env that referenced this issue Apr 20, 2024
as it is actually dangerous (https://hackerone.com/reports/678487), and its status is not likely to be resolved (nodejs/node#42232, nodejs/node#12682)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants