url: added url.resolve() method's behaviour #8969
Conversation
nodejs-github-bot
added
doc
| @@ -234,6 +234,16 @@ url.resolve('/one/two/three', 'four') // '/one/two/four' | |||
| url.resolve('http://example.com/', '/one') // 'http://example.com/one' | |||
| url.resolve('http://example.com/one', '/two') // 'http://example.com/two' | |||
| ``` | |||
| Note that `url.resolve()` method behaves differently depending on the protocol given in the `from` parameter. Some protocols like https, file, ftp, wss and gopher have special cases. | |||
There was a problem hiding this comment.
A couple of suggestions:
- Can you leave a blank line above this one?
- Can you try to wrap the line after 80 characters?
- I’d suggest enclosing the protocol names in backticks, i.e.
https, that seems a bit more readable to me.
| ```js | ||
| url.resolve('https://foo.tld', 'bar'); // 'https://foo.tld/bar' | ||
| url.resolve('wss://foo.tld', 'bar') // 'wss://bar' | ||
| url.resolve('ftps://foo.tld', 'bar') // 'ftps://bar' |
There was a problem hiding this comment.
I don’t have a strong preference for either way, but probably either all of these lines should end with a semicolon or none of them?
| url.resolve('wss://foo.tld', 'bar') // 'wss://bar' | ||
| url.resolve('ftps://foo.tld', 'bar') // 'ftps://bar' | ||
| ``` | ||
| Although,the uniform behaviour of the method is achieved when the URL ends with a slash. |
There was a problem hiding this comment.
There’s a space missing after Although, (and maybe a blank line above this one, too). Also, it might be good to have an example of that behaviour listed here?
|
The subsystem should be |
|
@minervapanda I think we always use |
|
Yep, what @gibfahn said. |
|
I will take care of this and amend the commit here. Thanks for the clarification. |
minervapanda
force-pushed
the
master
branch
4 times, most recently
from
3c75eea to
e33fdcd
Compare
| @@ -235,6 +235,18 @@ url.resolve('http://example.com/', '/one') // 'http://example.com/one' | |||
| url.resolve('http://example.com/one', '/two') // 'http://example.com/two' | |||
| ``` | |||
|
|
|||
| Note that `url.resolve()` method depends on the protocol given in the `from` parameter. Some protocols like `https`, `file`, `ftp`, `wss` and `gopher` have special cases. | |||
There was a problem hiding this comment.
oh, one additional minor nit: can you please line wrap at 80-chars?
There was a problem hiding this comment.
Yeah sure. Eliminating this line " Note that url.resolve() method depends on the protocol given in the from parameter." Would be fine right ?
There was a problem hiding this comment.
That first sentence is fine, just add a line break or two in there so a single line doesn't go past 80 chars :-)
minervapanda
force-pushed
the
master
branch
2 times, most recently
from
558bdd4 to
7fe931c
Compare
|
If I'm not mistaken, it looks like this adds four files that shouldn't be there. This should just be the changes to |
|
Tiny nit: The verb ( |
|
Yes. @Trott Everything was okay in the previous commit.While committing again , by mistake I changed 4 other files. I am fixing this now. I am puzzled , what are those other files. |
minervapanda
force-pushed
the
master
branch
2 times, most recently
from
873871b to
c0e1785
Compare
|
Yeah, this looks good. |
|
@minervapanda Any specific reason to close this? You may want to remove the last commit here but otherwise this PR was just fine. |
|
|
|
@minervapanda use a different branch for each issue/PR. From your fork of node run this for each PR you want to submit: Do your changes, then commit and send a PR from that branch. |
Checklist
Affected core subsystem(s)
url
Description of change
Solved #8921 - Updated documentation for url.resolve() by mentioning about how the method behaves differently with the change of protocol in the "from" parameter.