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

url.resolve() behaves differently depending on the protocol of the base url #8921

Closed
rolinh opened this issue Oct 4, 2016 · 10 comments
Closed
Assignees
Labels
doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module.

Comments

@rolinh
Copy link

rolinh commented Oct 4, 2016

  • Version: v4.5.0, v6.7.0
  • Platform: Linux 3.16.0-76-generic x86_64, Linux 4.7.6-1-ARCH x86_64, FreeBSD 10.1-RELEASE-p26 amd64
  • Subsystem: url

The url.resolve() function behaves differently depending on the protocol given in the from parameter.
See this example:

> var url = require("url");
undefined
> 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'

When reading this function's documentation, this is not a behavior that I could foresee. I would have expected 'wss://foo.tld/bar' and 'ftps://foo.tld/bar' as results of the last two calls respectively.

@bnoordhuis bnoordhuis added the url Issues and PRs related to the legacy built-in url module. label Oct 4, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 4, 2016

Some protocols (http, https, file, ftp and, yes, gopher) are treated specially, to match browser behavior ca. 2010, when the url module was introduced.

I think it's confusing but it's probably hard to change because of backwards compatibility. It's more likely to simply be subsumed by WHATWG URL support, ref #7448.

EDIT: Forgot to mention, if the URLs end in a slash, you get uniform behavior. I'll add documentation tags.

@bnoordhuis bnoordhuis added doc Issues and PRs related to the documentations. docs-requested labels Oct 4, 2016
@rolinh
Copy link
Author

rolinh commented Oct 4, 2016

Thanks @bnoordhuis. I believe that the information you give in your comment (the list of protocols which are treated specially and the uniform behavior when the URL ends with a slash) definitely has its place in the documentation.

@evanlucas evanlucas added the good first issue Issues that are suitable for first-time contributors. label Oct 4, 2016
@fhinkel fhinkel self-assigned this Oct 6, 2016
@fhinkel
Copy link
Member

fhinkel commented Oct 6, 2016

Assigned to myself as placeholder for @minervapanda

@minervapanda
Copy link

minervapanda commented Oct 7, 2016

According to my knowledge , the issue is pointing to the url.md to the following lines of url.resolve() : 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'

It looks good to me. Please correct me if I am wrong.

@rolinh
Copy link
Author

rolinh commented Oct 7, 2016

@minervapanda Yes, the issue is pointing to these lines and the examples are correct. However, the documentation does not tell that:

  • Some protocols are treated as special cases (http, https, file, ftp and gopher).
  • The way in which these protocols are treated differently.
  • The behavior of url.resolve() differs whether the URL ends with a / or not.

@minervapanda
Copy link

Thank you for the clarification @rolinh . I was confused ,the example seemed correct.I will make a PR to update the documentation.Thanks.

@addaleax addaleax removed the good first issue Issues that are suitable for first-time contributors. label Oct 7, 2016
minervapanda added a commit to minervapanda/node that referenced this issue Oct 11, 2016
@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. docs-requested labels Dec 1, 2016
@danbeam
Copy link

danbeam commented Feb 1, 2017

I'm optimizing the new version of Chrome's settings UI written in Polymer.

The node-based tool I was using (vulcanize) calls url.resolve() a bunch and wasn't working well for our funky chrome:// URLs (the settings page lives at chrome://settings). We ended up needing to shift all our URLs around (i.e. src= for <script>, href= for <link>) to actually get things working.

I tracked it back to unexpected results with url.resolve() in node (we're using v6.9.2):

> require('url').resolve('chrome://settings', '/page.html')
chrome:///page.html

For what it's worth, I also messed around with adding a slash at the end of the from (first) argument, but that doesn't always work as we expected either:

> require('url').resolve('chrome://settings/', '/page.html')
chrome:///page.html

I expected chrome://settings/page.html if that wasn't clear. And yes: a slash at the end of from and not at the beginning of to does work, but it's not the same (doesn't resolve correctly for all URLs).

When i change the protocol, I get more expected results:

> require('url').resolve('http://chrome.tld', '/blah.html')
http://chrome.tld/blah.html

I assume this has to do with the whitelist of protocols in lib/url.js. Certainly things could be added to this list, but the returns are diminishing.

I also found this todo:

// v0.12 TODO(isaacs): This is not quite how Chrome does things.

This got me thinking: is node interested in using an implementation from an open source browser like KURL from chromium? This is the technology behind the URL constructor exposed to DOM.

/cc @arv as an FYI

@jasnell
Copy link
Member

jasnell commented Feb 1, 2017

The current direction is to leave the existing url.parse() and url.resolve() behavior as is and focus efforts into the new WHATWG URL implementation that is much more robust, correct, and complete in it's handling of URLs. The implementation matches (as closely as currently possible) the behavior and API available in browsers.

@TimothyGu
Copy link
Member

@danbeam, indeed, in Node.js v7 you can already do:

const { URL } = require('url');
console.log(new URL('/page.html', 'chrome://settings/').href);
  // Prints 'chrome://settings/page.html'

Or if Node.js v7 is too high of a requirement, @domenic's whatwg-url would also work.

@Trott
Copy link
Member

Trott commented Jul 16, 2017

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

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. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.