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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions doc/api/url.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,28 @@ manner similar to that of a Web browser resolving an anchor tag HREF.
For example:

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Every other (for instance ftps or wss) is treated differently whether its URL
Copy link
Contributor

Choose a reason for hiding this comment

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

What is different? The docs don't say.

ends in a slash or not.
Copy link
Member

Choose a reason for hiding this comment

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

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:


For example:

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

Although, the uniform behaviour of the method is achieved when the URL ends with a slash.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the uniform behaviour?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


```js
url.resolve("ftps://foo.tld/", "bar"); // 'ftps://foo.tld/bar'
```
## Escaped Characters

URLs are only permitted to contain a certain range of characters. Spaces (`' '`)
Expand Down