doc: add URL.format() example - #18888
Conversation
Sorry, something went wrong.
| Example: | ||
|
|
||
| ```js | ||
| const URL = require('url'); |
There was a problem hiding this comment.
Although it appears in other examples in this doc, I think in general we tend to leave out the require() for the module. I'm fine either way.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh! But we do use url and reserve URL for the newer url.URL. So maybe remove it and use url rather than URL below.
Sorry, something went wrong.
|
LGTM once we get the URL/url stuff sorted out. |
Sorry, something went wrong.
|
Took out the require and changed URL to url to avoid new API / old API confusion. Thanks for the quick feedback! |
Sorry, something went wrong.
| The `url.format()` method returns a formatted URL string derived from | ||
| `urlObject`. | ||
|
|
||
| Example: |
There was a problem hiding this comment.
Nit: You can remove Example: too. It's obvious that it is an example.
Sorry, something went wrong.
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM but I think it would be good to switch to "Prints".
Sorry, something went wrong.
| } | ||
| }); | ||
|
|
||
| // => 'https://example.com/some/path?page=1&format=json' |
There was a problem hiding this comment.
Other places in the file use Prints or just show the output below. I personally think a "Prints" would be better than =>.
Sorry, something went wrong.
There was a problem hiding this comment.
I did a search in the docs and found several instances of // => symbolizing a returned value. Also, this method doesn't actually print a value, it just returns it.
Sorry, something went wrong.
There was a problem hiding this comment.
I would be open to just doing // https://.... That was another "convention" I found when looking at existing code examples in Node's docs.
Sorry, something went wrong.
There was a problem hiding this comment.
In this case I only spoke about this specific document. And yes, // => is still used some times but it is not really common as far as I know.
So I personally would say using // 'https:... would be good.
Sorry, something went wrong.
| ```js | ||
| url.format({ | ||
| protocol: 'https', | ||
| hostname: `example.com`, |
There was a problem hiding this comment.
Are the backticks intended? If so, are not they a bit confusing here? With them, a template literal/a tag function/other quotes inside are usually expected.
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch, @vsemozhetbyt. Backticks were not intentional there. Fixed.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #18888 Fixes: #18887 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: nodejs#18888 Fixes: nodejs#18887 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: nodejs#18888 Fixes: nodejs#18887 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: nodejs#18888 Fixes: nodejs#18887 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Backport-PR-URL: #22380 PR-URL: #18888 Fixes: #18887 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
This PR adds an example use of the URL.format() method.
Resolves #18887
Checklist
Affected core subsystem(s)