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

url: docs deprecate legacy url API #22715

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 5, 2018

It's time to start the deprecation process for the legacy URL API.

Documentation-only deprecation for Node.js 11
Hopefully upgraded to Runtime-deprecation in Node.js 12

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. labels Sep 5, 2018
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 5, 2018
@jasnell jasnell requested a review from a team September 5, 2018 18:36
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

The functions should be marked as deprecated in url.md.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

👍 on process

@jasnell
Copy link
Member Author

jasnell commented Sep 5, 2018

@targos ... done!

@vsemozhetbyt vsemozhetbyt added the url Issues and PRs related to the legacy built-in url module. label Sep 5, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Hopefully upgraded to Runtime-deprecation in Node.js 12

That time frame seems off by a few years?

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

Would it be to early to add this to the pending deprecations? That would allow people who actively opt in to know that it's deprecated even without looking at the docs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Unfortunately, I see #12682 as a blocker for the deprecation of the legacy URL API. We need to have a story for Express and other modules that still heavily use the legacy API (Express does so through the parseurl wrapper), whether that be a userland module or a Core API, preferably before we start the deprecation proceedings and definitely before we runtime-deprecate the API. Sorry about blocking this.

@jasnell
Copy link
Member Author

jasnell commented Sep 5, 2018

@TimothyGu, that's precisely why this is a documentation only deprecation.

@jasnell
Copy link
Member Author

jasnell commented Sep 5, 2018

In other words, I don't think we can actually start moving people through the migration until we start the deprecation because there won't be pressure to actually change

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

With the commitment to address #12682 before runtime deprecation, LGTM.

Type: Documentation

The [Legacy URL API][] is deprecated in favor of the newer standardized
[WHATWG URL][] implementation. This includes uses of `url.format()`,
Copy link
Member

Choose a reason for hiding this comment

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

url.format() should be qualified to not include the overload that takes URL objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it should. I added that strictly as a convenience. The intent here is to move people away from the non-standard API and keeping that one variant un-deprecated would not make sense. Folks really should be using the href property on the URL object instead.


The [Legacy URL API][] is deprecated in favor of the newer standardized
[WHATWG URL][] implementation. This includes uses of `url.format()`,
`url.parse()`, `url.resolve()`, and the legacy `urlObject`.
Copy link
Member

Choose a reason for hiding this comment

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

Links for the functions and "legacy urlObject" would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added :-)

@jasnell jasnell force-pushed the deprecate-legacy-url branch from 66e8a36 to 3a04026 Compare September 6, 2018 14:45

The [Legacy URL API][] is deprecated in favor of the newer standardized
[WHATWG URL][] implementation. This includes uses of [`url.format()`][],
[`url.parse()`][], [`url.resolve()`][], and the [legacy `urlObject`][].
Copy link
Member

@Trott Trott Sep 7, 2018

Choose a reason for hiding this comment

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

@jasnell Is it OK with you if I change the text to this?:

The [Legacy URL API][] is deprecated. This includes [`url.format()`][],
[`url.parse()`][], [`url.resolve()`][], and the [legacy `urlObject`][]. Please
use the [WHATWG URL API][] instead.

(This will also require updating the link reference currently on line 1158 to say WHATWG URL API rather than WHATWG URL.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Whatever you'd like

jasnell added a commit that referenced this pull request Sep 7, 2018
PR-URL: #22715
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Sep 7, 2018

Landed in 922a1b0

@jasnell jasnell closed this Sep 7, 2018
@tniessen tniessen mentioned this pull request Sep 8, 2018
3 tasks
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 9, 2018
PR-URL: nodejs#22765
Refs: nodejs#22715
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Fishrock123
Copy link
Contributor

I do not agree with deprecating url.format() with no suitable replacement: #25099

@jasnell
Copy link
Member Author

jasnell commented Dec 20, 2018

Please define "suitable replacement" as that is quite subjective. The URL standard does expose ways of creating and formatting a URL that would be relatively trivial to wrap into a url.format() type of utility function is userland. Also note, the deprecation is docs-only, and was signed off by 11 collaborators (5 of whom are TSC). There were no objections prior to the deprecation landing. I see no reason to remove the deprecation for url.format()

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. doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.