url: fix/8722

[yorkie]

This patch attempts to close this issue #8722:

• feature
• test
• documentation
• fix some style conflicts with jshint in test-url.js

R=@indutny, @chrisdickinson 🍒

[indutny]

Could we handle it without creating closure?

[yorkie]

Removed closure function 🌸

[chrisdickinson]

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

[yorkie]

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"?

[yorkie]

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?

[chrisdickinson]

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").

[yorkie]

ah, gotcha, i was just getting it backwards!!!
Fixed now, please review again @chrisdickinson thank you

[chrisdickinson]

I think there's one last case to check here -- path is present, but has no ?..., so the output url should have no ?... -- i.e., the query, search, and pathname keys are ignored if path is given.

[yorkie]

@chrisdickinson reset this.query and this.set for now :)

[chrisdickinson]

It should probably read "path wins" here and on line 1154 now -- but I can fix that up on merge.

[chrisdickinson]

This is great. Thanks! LGTM /cc @indutny, if nobody has any qualms, I'll merge this tomorrow evening.

[yorkie]

Fixed comments now
/cc @chrisdickinson @indutny 🍒

[yorkie]

ping @chrisdickinson 🎅

[yorkie]

struggle for pinging @chrisdickinson (This PR should has such a long delay?)

[chrisdickinson]

Ack! Sorry this took so long for me to pick up again. Looking now.

[chrisdickinson]

Thanks for your patience! Merged in d312b6d.

[yorkie]

np 🍒