Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

url: fix/8722 #8755

Closed
wants to merge 4 commits into from
Closed

url: fix/8722 #8755

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
45f60bc
url: support `path` for url.format
yorkie Nov 20, 2014
cd5b060
doc: update for url.format with path
yorkie Nov 20, 2014
38e9dea
url: remove closure
yorkie Nov 20, 2014
da8c663
url: add more tests for url.format with path field
yorkie Nov 22, 2014
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
9 changes: 5 additions & 4 deletions doc/api/url.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,12 @@ Take a parsed URL object, and return a formatted URL string.
* `hostname` will only be used if `host` is absent.
* `port` will only be used if `host` is absent.
* `host` will be used in place of `hostname` and `port`
* `pathname` is treated the same with or without the leading `/` (slash)
* `search` will be used in place of `query`
* `pathname` is treated the same with or without the leading `/` (slash).
* `path` is treated the same with `pathname` but able to contain `query` as well.
* `search` will be used in place of `query`.
* `query` (object; see `querystring`) will only be used if `search` is absent.
* `search` is treated the same with or without the leading `?` (question mark)
* `hash` is treated the same with or without the leading `#` (pound sign, anchor)
* `search` is treated the same with or without the leading `?` (question mark).
* `hash` is treated the same with or without the leading `#` (pound sign, anchor).

## url.resolve(from, to)

Expand Down
10 changes: 10 additions & 0 deletions lib/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,16 @@ Url.prototype.format = function() {
host = false,
query = '';

if (this.path) {
var qm = this.path.indexOf('?');
if (qm !== -1) {
query = this.path.slice(qm);
pathname = pathname || this.path.slice(0, qm);
} else {
pathname = pathname || this.path;
Copy link

Choose a reason for hiding this comment

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

If path is given:

  1. if it contains "?...", that replaces the current query
  2. if it does not contain "?...", the current query is used
  3. if search is given, it always wins
  4. if hash is given, it always wins

I suppose my confusion is: for some attributes, the more specific attribute "wins" versus the more general attribute, while in the "path" case, the less specific version wins:

query is more specific than search is more specific than path -> search wins
pathname is more specific than path -> path wins
hash is more specific than path -> hash wins

Copy link
Author

Choose a reason for hiding this comment

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

pathname is more specific than path -> path wins
hash is more specific than path -> hash wins

hey @chrisdickinson they are not consistent because you say:

the less specific version wins

i guess in hash line, it should be "path wins"?

Copy link
Author

Choose a reason for hiding this comment

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

and for given pseudocode by last last comment, in path case, we should only touch one of query or search, because in path they are overlapped, so i choose query, search will be reassigned next code, so i propose the program will be:

  1. if it contains "?...", than replaces the current query
  2. if it doesn't, the current query is used of course included search
  3. if it contains "#...", then replaces the current hash
  4. if it doesn't, the current hash is used

query is more specific than search is more specific than path -> search wins

from my above statement, i guess we shouldn't touch search any more, just follow the step 1.
@chrisdickinson how do you think about my idea?

Copy link

Choose a reason for hiding this comment

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

Hm. I think we should try to mirror how host (vs. hostname + port) operates with path -- that is:

If path is given, it is assumed to include search/query and pathname. If those are not present in path, and path is given, then they will be omitted from the output of format.

So:

// we're matching this behavior: (port is omitted from "host" but "host" overrides port)
assert.equal(url.format({host: 'some.host.com', hostname: 'example.com', port: 8080}), '//some.host.com');
assert.equal(url.format({hostname: 'example.com', port: 8080}), '//example.com:8080');

// "auth" is not considered part of "host"
assert.equal(url.format({auth: 'a:b', host: 'some.host.com', hostname: 'example.com', port: 8080}), '//a:[email protected]');
assert.equal(url.format({auth: 'a:b', hostname: 'example.com', port: 8080}), '//a:[email protected]:8080');

// "path" does not include "hash", but does include "search" and "pathname"
assert.equal(url.parse('http://example.com/path/to/thing?q=1#hash').path, '/path/to/thing?q=1')
// "path" overrides pathname
assert.equal(url.format({host: "example.com", path: "/path/to/thing?q=1", pathname: "/anythingelse"}), '//example.com/path/to/thing?q=1') 
// "path" overrides search/query
assert.equal(url.format({host: "example.com", path: "/path/to/thing?q=1", search: "?p=2"}), '//example.com/path/to/thing?q=1')
// "path" overrides search/query even if not given in "path"
assert.equal(url.format({host: "example.com", path: "/path/to/thing", search: "?p=2"}), '//example.com/path/to/thing')
// similarly, "path" overrides "pathname" even if not given:
assert.equal(url.format({host: "example.com", path: "?p=2", pathname: "/anything"}), '//example.com?p=2')

to illustrate:

+-----------------------------------------------------------------------------------+
| href                                                                              |
+----------+--+-----------+--------------------+-----------------------------+------+
| protocol |* | auth      | host               | path                        | hash |
|          |  |           +-------------+------+----------+------------------+      |
|          |  |           | hostname    | port | pathname | search           |      |
|          |  |           |             |      |          +-+----------------+      |
|          |  |           |             |      |          | | query          |      |
" http:     //   user:[email protected]:65535 /path/name ? search=1&continues#hash "
|          |  |           |             |      |          | |                |      |
+----------+--+-----------+-------------+------+----------+-+----------------+------+
(all spaces in the "" line should be ignored -- they're purely for formatting)
*: given by "slashes" key

Setting a key at the higher level should cause url.format to ignore all keys at a "lower" level, whether or not that lower key is present in the text of the higher level key (even though "port" may not be present in "host", setting "host" ignores all values for "port").

}
}

if (this.host) {
host = auth + this.host;
} else if (this.hostname) {
Expand Down
17 changes: 13 additions & 4 deletions test/simple/test-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ var formatTests = {

// `#`,`?` in path
'/path/to/%%23%3F+=&.txt?foo=theA1#bar' : {
href : '/path/to/%%23%3F+=&.txt?foo=theA1#bar',
href: '/path/to/%%23%3F+=&.txt?foo=theA1#bar',
pathname: '/path/to/%#?+=&.txt',
query: {
foo: 'theA1'
Expand All @@ -1095,7 +1095,7 @@ var formatTests = {

// `#`,`?` in path + `#` in query
'/path/to/%%23%3F+=&.txt?foo=the%231#bar' : {
href : '/path/to/%%23%3F+=&.txt?foo=the%231#bar',
href: '/path/to/%%23%3F+=&.txt?foo=the%231#bar',
pathname: '/path/to/%#?+=&.txt',
query: {
foo: 'the#1'
Expand All @@ -1110,7 +1110,7 @@ var formatTests = {
hostname: 'ex.com',
hash: '#frag',
search: '?abc=the#1?&foo=bar',
pathname: '/foo?100%m#r',
pathname: '/foo?100%m#r'
},

// `?` and `#` in search only
Expand All @@ -1120,7 +1120,16 @@ var formatTests = {
hostname: 'ex.com',
hash: '#frag',
search: '?abc=the#1?&foo=bar',
pathname: '/fooA100%mBr',
pathname: '/fooA100%mBr'
},

// only for path rather than pathname
'http://github.com/joyent/node#js': {
href: 'http://github.com/joyent/node#js',
protocol: 'http:',
hostname: 'github.com',
hash: '#js',
path: '/joyent/node'
}
Copy link
Author

Choose a reason for hiding this comment

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

Hey @chrisdickinson, please review the above 5 tests from here, it follows your diagram and description: higher than lower. btw, thanks for your such detailed diagram, that's helpful and super awesome 🍒

};
for (var u in formatTests) {
Expand Down