doc: add url.resolve() method's behaviour #8991

minervapanda · 2016-10-09T14:51:16Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

url

Description of change

url.resolve() behaviour depends on the protocol in the "from" parameter of the method.

addaleax · 2016-10-09T14:54:36Z

For the record, this continues from #8969 with sign-offs from me, @jasnell and @lpinca for the commit that is here, and a request to use add in the commit message.

Also, this fixes #8921.

minervapanda · 2016-10-09T14:56:54Z

@addaleax Done :)

addaleax · 2016-10-09T15:07:59Z

@minervapanda As far as I can see, what you’ve updated is the PR title, not the commit message itself (you can see that listed here).

Changing the commit message requires a bit of git magic, and if you don’t feel up for that, don’t worry – something like that can be performed by the person who’s merging the PR, so you shouldn’t feel obligated to do anything.

But if you want to give it a try:

Check out your pr1 branch (I assume you have named it that way)
Make sure that the commit at the head of the branch is the one you want to edit, e.g. using git show
Edit the commit message using git commit --amend. That should give you an editor* and the way to change the message in whatever way you want
Add the add verb to the commit message, and ideally add a last line that says Fixes: https://github.com/nodejs/node/issues/8921
Once you’re happy with the changes, save the file and exit the editor.
Make sure the commit message looks like you want it to be like – again, using git show
If everything looks fine, push to this PR using git push --force-with-lease origin p1.

This may seem tricky (and it’s certainly complicated) – if you have any questions, don’t hesitate to ask!

(* Depending on your OS/system setup the editor may vary; if you end up in vim and can’t work with that (which I definitely can’t 😄), it might help to enter EDITOR=nano in the shell beforehand.)

lpinca · 2016-10-09T15:22:43Z

doc/api/url.md

+
+```js
+url.resolve('https://foo.tld', 'bar')   // 'https://foo.tld/bar'
+url.resolve('wss://foo.tld', 'bar')      // 'wss://bar'


Nit: very very tiny, feel free to ignore it. Can you align the comment with the one above? Same for the following line.

@lpinca Please keep mentioning even the tiniest of mistakes I do. I am a beginner,I am learning a lot in this process.
I am going to fix all that you said.

While you are at it also add semicolons mostly for consistency with other code example in the doc.

lpinca

LGTM

lpinca · 2016-10-09T15:29:23Z

doc/api/url.md

@@ -235,6 +235,19 @@ 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. 


Nit: Can you please split lines longer than 80 chars over multiple lines? Thanks!

minervapanda · 2016-10-09T15:37:03Z

Wow! @addaleax ,you explained so simply :) Thanks a lot for making Git so simple for me. I understood what I was doing.

I got stuck in the last step , Everything worked until I ran git push --force-with-lease origin p1 which showed :
error: src refspec p1 does not match any.
error: failed to push some refs to 'https://github.com/minervapanda/node.git'

addaleax · 2016-10-09T15:40:34Z

@minervapanda oh, yeah, that’s a typo, I meant pr1, not p1. 😄

rolinh

I'm sorry to intervene here but I have minor suggested changes for this PR. Using "like" about the special case protocols make it sound vague although the list is very well defined.
Thanks for this update!

rolinh · 2016-10-10T05:49:56Z

doc/api/url.md

@@ -235,6 +235,19 @@ 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.


The list of protocols that are treated as special cases is very precise and actually defined as this list (see const slashedProtocol in lib/url.js) :

http

https

ftp

gopher

file

So protocol wss is not treated as a special case (as well as ftps or any other). I would change the sentence for something like this:

Some protocols (http, https, ftp, gopher and file) are treated as special cases. Every other (for instance ftps or wss) is treated differently whether its URL ends in a slash or not. Although, a uniform behaviour of the method is achieved when the URL ends with a slash.

(note that I moved the last sentence up here)

Hi, Sure I am making the changes.

rolinh · 2016-10-10T05:52:04Z

doc/api/url.md

+```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'


To follow on my previous comment, I think it would be neat to include an example with a trailing / in a URL. Like adding this example to your list:

url.resolve("ftps://foo.tld/", "bar"); // 'ftps://foo.tld/bar'

Fixes: nodejs#8921

lpinca · 2016-10-11T11:20:28Z

doc/api/url.md

 ```

+Some protocols (`http`, `https`, `ftp`, `gopher` and `file`) are treated as special cases.


Can you add a line break at column <= 80? This applies for all lines not only this one.
Thanks!

minervapanda · 2016-11-02T10:19:34Z

@addaleax when can this be merged to the master ?

addaleax

Thanks for the bump! This looks good to me with @lpinca’s nit addressed, but that can also happen when merging the PR as far as I’m concerned.

I’ll land this later today if nobody beats me to it/there are no objections.

sam-github · 2016-11-02T14:11:43Z

doc/api/url.md

 ```

+Some protocols (`http`, `https`, `ftp`, `gopher` and `file`) are treated as special cases.


What is special about them? The docs don't say.

It has been explained explicitly with the help of examples how the usual behaviour might differ taking into the consideration the different protocols

sam-github · 2016-11-02T14:13:10Z

doc/api/url.md

 ```

+Some protocols (`http`, `https`, `ftp`, `gopher` and `file`) are treated as special cases.
+Every other (for instance ftps or wss) is treated differently whether its URL 


What is different? The docs don't say.

sam-github · 2016-11-02T14:13:39Z

doc/api/url.md

+url.resolve('ftps://foo.tld', 'bar');    // 'ftps://bar'
+```
+
+Although, the uniform behaviour of the method is achieved when the URL ends with a slash.


What is the uniform behaviour?

#8057 Please go through the issue that has been addressed here.

Sorry, that isn't helpful - the point of the docs is to explain, and they aren't, so having me go read a node issue to understand a doc change is a bit backwards.

As is, this PR adds some extra examples and some lines of text that basically say "look carefully at the examples to see how this API works, paying particular attention to the protocol scheme, because it changes the behaviour". This is arguably an improvement over the existing doc, if @addaleax is happy with that, I've no objection.

However, it still doesn't document the API behaviour. If you have the interest, the existing docs could be improved even more. If not, I understand.

This is arguably an improvement over the existing doc

Yeah, that’s what my approval of this PR is based upon. I don’t disagree with you that this still leaves room for improvement.

sam-github · 2016-11-02T15:29:01Z

This PR doesn't document the behaviour, it adds examples, and leaves it up to the user to try to figure out what the behaviour is by reading the examples. Is it possible to actually describe in words what the behaviour is?

jasnell · 2016-11-03T15:00:18Z

doc/api/url.md

 ```

+Some protocols (`http`, `https`, `ftp`, `gopher` and `file`) are treated as special cases.
+Every other (for instance ftps or wss) is treated differently whether its URL 
+ends in a slash or not.


I would suggest wording this a bit differently:

The specific result of resolving a URL depends on the protocol scheme. URL's using protocol `http://` or `https://`, for instance, resolve differently than URLs using protocol `wss:`, as illustrated in the example below:

italoacasas · 2017-01-16T21:31:35Z

ping @minervapanda

fhinkel · 2017-05-26T19:11:36Z

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

nodejs-github-bot added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels Oct 9, 2016

minervapanda changed the title ~~doc: url.resolve() method's behaviour~~ doc: add url.resolve() method's behaviour Oct 9, 2016

addaleax mentioned this pull request Oct 9, 2016

doc: special cases in http module response body #8992

Closed

2 tasks

lpinca reviewed Oct 9, 2016

View reviewed changes

lpinca approved these changes Oct 9, 2016

View reviewed changes

lpinca reviewed Oct 9, 2016

View reviewed changes

minervapanda force-pushed the pr1 branch from 4c58202 to b433b01 Compare October 9, 2016 17:28

rolinh suggested changes Oct 10, 2016

View reviewed changes

minervapanda force-pushed the pr1 branch 2 times, most recently from 36120b2 to 7f93629 Compare October 11, 2016 10:03

doc: add url.resolve() method's behaviour

94698bd

Fixes: nodejs#8921

minervapanda force-pushed the pr1 branch from 7f93629 to 94698bd Compare October 11, 2016 10:07

lpinca reviewed Oct 11, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

addaleax approved these changes Nov 2, 2016

View reviewed changes

sam-github reviewed Nov 2, 2016

View reviewed changes

jasnell reviewed Nov 3, 2016

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel closed this May 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add url.resolve() method's behaviour #8991

doc: add url.resolve() method's behaviour #8991

minervapanda commented Oct 9, 2016

addaleax commented Oct 9, 2016

minervapanda commented Oct 9, 2016

addaleax commented Oct 9, 2016

lpinca Oct 9, 2016

minervapanda Oct 9, 2016

lpinca Oct 9, 2016

lpinca left a comment

lpinca Oct 9, 2016

minervapanda commented Oct 9, 2016

addaleax commented Oct 9, 2016

rolinh left a comment

rolinh Oct 10, 2016 •

edited

Loading

minervapanda Oct 11, 2016

rolinh Oct 10, 2016 •

edited

Loading

lpinca Oct 11, 2016

minervapanda commented Nov 2, 2016

addaleax left a comment

sam-github Nov 2, 2016

minervapanda Nov 2, 2016

sam-github Nov 2, 2016

sam-github Nov 2, 2016

minervapanda Nov 2, 2016

sam-github Nov 2, 2016

addaleax Nov 2, 2016

sam-github commented Nov 2, 2016

jasnell Nov 3, 2016

italoacasas commented Jan 16, 2017

fhinkel commented May 26, 2017

		```

		Some protocols (`http`, `https`, `ftp`, `gopher` and `file`) are treated as special cases.

doc: add url.resolve() method's behaviour #8991

doc: add url.resolve() method's behaviour #8991

Conversation

minervapanda commented Oct 9, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax commented Oct 9, 2016

minervapanda commented Oct 9, 2016

addaleax commented Oct 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minervapanda commented Oct 9, 2016

addaleax commented Oct 9, 2016

rolinh left a comment

Choose a reason for hiding this comment

rolinh Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolinh Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minervapanda commented Nov 2, 2016

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Nov 2, 2016

Choose a reason for hiding this comment

italoacasas commented Jan 16, 2017

fhinkel commented May 26, 2017

rolinh Oct 10, 2016 •

edited

Loading

rolinh Oct 10, 2016 •

edited

Loading