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

Documentation for http.message.url suggests using deprecated url API #30048

Closed
denilsonsa opened this issue Oct 21, 2019 · 8 comments
Closed
Labels
doc Issues and PRs related to the documentations.

Comments

@denilsonsa
Copy link

At the following piece of documentation (for v12; the latest one):

It describes:

To parse the url into its parts require('url').parse(request.url) can be used:
[…]
…or true can be passed as the second argument to require('url').parse:
[…]

However, this legacy url module/API is deprecated since v11.

Thus, the documentation should be updated.

Bonus points for grepping the rest of the documentation to find other outdated examples.

@lpinca
Copy link
Member

lpinca commented Oct 21, 2019

The WHATWG URL API will not work in this case unless a bogus base URL is provided as second argument.

@lpinca lpinca added the doc Issues and PRs related to the documentations. label Oct 21, 2019
@saitolume
Copy link
Contributor

@lpinca Can I work on this issue?
This is my first time contributing to Node.js

@lpinca
Copy link
Member

lpinca commented Nov 12, 2019

Yes but again, there is no equivalent for WHATWG URL.

@saitolume
Copy link
Contributor

saitolume commented Nov 12, 2019

Hmm… Should I remove deprecated docs or update with WHATWG URL to provide a bogus URL as second argument?

If the later, I will replace it like this:

> new URL('/status?name=ryan', 'https://nodejs.org')
URL {
  href: 'https://nodejs.org/status?name=ryan',
  origin: 'https://nodejs.org',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'nodejs.org',
  hostname: 'nodejs.org',
  port: '',
  pathname: '/status',
  search: '?name=ryan',
  searchParams: URLSearchParams { 'name' => 'ryan' },
  hash: ''
}

@lpinca
Copy link
Member

lpinca commented Nov 15, 2019

@nodejs/url

@dev-script
Copy link
Contributor

@lpinca i would like to work on this issue.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 5, 2019

In the context of http.IncomingMessage, it might make sense to suggest something like

new URL(request.url, `http://${request.headers.host}`);

(or http:// can be replaced with a ${protocol}:// to be more generic. Of course this does not work if request.headers does not actually contain host but I guess in that case a 404 should already be sent anyway)

Then give some examples like the one in #30048 (comment)

@saitolume
Copy link
Contributor

@joyeecheung Thank you for a good idea! I will try to refactor doc soon.

saitolume added a commit to saitolume/node that referenced this issue Dec 7, 2019
Update message.url example to use The WHATWG URL API.
This is because the old example suggests using deprecated url API.

Fixes: nodejs#30048
Refs: https://nodejs.org/dist/latest-v12.x/docs/api/http.html#http_message_url
saitolume added a commit to saitolume/node that referenced this issue Dec 8, 2019
Wrapped at 80 characters and refactored.

Fixes: nodejs#30048
Refs: nodejs#30830
@Trott Trott closed this as completed in eaf5975 Dec 18, 2019
BridgeAR pushed a commit that referenced this issue Jan 3, 2020
Update message.url example to use The WHATWG URL API.
This is because the old example suggests using deprecated url API.

Fixes: #30048
PR-URL: #30830
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this issue Jan 14, 2020
Update message.url example to use The WHATWG URL API.
This is because the old example suggests using deprecated url API.

Fixes: #30048
PR-URL: #30830
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
Update message.url example to use The WHATWG URL API.
This is because the old example suggests using deprecated url API.

Fixes: #30048
PR-URL: #30830
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants