url: re-add path in url.format

[yorkie]

This PR re-added the commit d312b6d that reverted at #303, and in this time, I did cover the test with the case git+ssh://[email protected]:<user>/<repo> and let it passed:

> url.parse('git+ssh://[email protected]:iojs/io.js.git')
{ protocol: 'git+ssh:',
  slashes: true,
  auth: 'git',
  host: 'github.com',
  port: null,
  hostname: 'github.com',
  hash: null,
  search: null,
  query: null,
  pathname: '/:iojs/io.js.git',
  path: '/:iojs/io.js.git',
  href: 'git+ssh://[email protected]/:iojs/io.js.git' }

R= @bnoordhuis @evanlucas /cc @rvagg

[piscisaureus]

node.js has rejected this patch because it could not decide what to do when the object contains both 'path' and 'pathname'. I don't think that iojs has answered that question, although the tests seem to contain some rules.

[rvagg]

my vote would be top prioritise path if it exists as it represents a parent to pathname and querystring and could be thought of as a parent - I say we just come up with decision about which is authoritative and be done with it.

[yorkie]

node.js has rejected this patch because it could not decide what to do when the object contains both 'path' and 'pathname'.

@piscisaureus @chrisdickinson has merged the patch at nodejs/node-v0.x-archive#8755, and we have discussed how the path acts as in the format() and parse() at nodejs/node-v0.x-archive#8755 (comment), it should be consistent with @rvagg's above vote.

Actually the most initial purpose of this patch is to keep url module consistent with http module as I described at nodejs/node-v0.x-archive#8722 that I talked with @indutny.

Btw, this issue is not big deal to me, I just wanna let the url in our internal modules be neater, so if you guys think it breaks too much, feel free to close it.

[piscisaureus]

Ok, the discussion in node seems to have been thorough enough so I move io accepts the patch as well.
The only thing that's needed is a semver range. What is this, semver-minor (added feature) ?

[yorkie]

Thank you @piscisaureus, as for semver-minor, I think minor is good, because I wasn't fixing anything else :)

[rvagg]

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/187/

[chrisdickinson]

This will be a major change in practice, unfortunately. Consider code that accepts a string, 'url.parse's it, changes the pathname, and formats it, before handing the string to another function. This change will break that behavior, because "path" will be present on the parsed url object and will take precedence over "pathname".

[chrisdickinson]

An example:

var url = require('url');

module.exports = myCode;

function myCode(sourceURL) {
  var parsed = url.parse(sourceURL);
  parsed.pathname = '/updated/by/proxy';
  return url.format(parsed);
}

One solution might be to (hurk) start introducing setters and getters on properties of the Url instances returned by url.parse, so that updating one key updates parent keys as well.

[yorkie]

Wow~ neat @chrisdickinson the pathname should be watched and effects on path, search and query fields, right?

Perhaps we are able to add the watching after the current build is passed, could someone please help me with failures of testing on https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/187/?

Actually I could not find a way to reproduce and moreover unsure if the errors is introduced by this patch because the same errors is also printed at the following builds:

• build 185: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/185/
• build 186: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/186/

/cc @rvagg

[rvagg]

you can ignore all those reds, freebsd and armv6 have persistent problems that are yet to be resolved

[yorkie]

Okay, thanks for your telling, will do watching by following @chrisdickinson's comment.

[yorkie]

Hi @rvagg @chrisdickinson added setter/getter and related tests, could someone please take a look, thanks :)

[rvagg]

@chrisdickinson can you semver-* label this? Do you still believe it to be semver-major?

[yorkie]

btw, I didn't introduce setter/getter on other fields like host/hostname/href/etc., so if this is tagged as major, perhaps I can do this with those fields, though :)

[yorkie]

ping @chrisdickinson

[chrisdickinson]

I'm not sure what the best course of action is, here. #933 also adds this behavior, and we're kind of at an impasse – a change like this stands to break a lot of code on npm, but it also makes the URL object how I expect it should. We should discuss this along with #933 at the next TC meeting.

[yorkie]

And actually when I run npm install git+ssh://[email protected]:caolan/async.git#master it still didn't work, but it worked at node v0.10.32.

So I ran the following:

# in node
> node
> url.parse('git+ssh://[email protected]:caolan/async.git#master');
{ protocol: 'git+ssh:',
  slashes: true,
  auth: 'git',
  host: 'github.com',
  port: null,
  hostname: 'github.com',
  hash: '#master',
  search: null,
  query: null,
  pathname: '/:caolan/async.git',
  path: '/:caolan/async.git',
  href: 'git+ssh://[email protected]/:caolan/async.git#master' }
# in iojs with mine
> node
> url.parse('git+ssh://[email protected]:caolan/async.git#master').data;
{ protocol: 'git+ssh:',
  slashes: true,
  auth: 'git',
  host: 'github.com',
  port: null,
  hostname: 'github.com',
  hash: '#master',
  href: 'git+ssh://[email protected]/:caolan/async.git#master',
  path: '/:caolan/async.git',
  pathname: '/:caolan/async.git',
  search: null,
  query: null }

So I guess as you have said, there are some changes on npm, let's waiting for that action, Thanks for your quick updates :)

[bnoordhuis]

What's the verdict on this PR? If it's acceptable for v2.x, can someone LGTM it?

[petkaantonov]

This looks like it would conflict a lot with the performance patch, so not LGTM :)

[yorkie]

I'm still waiting for that patch as well, i will fix conflicts once that would be merged

[Qard]

@nodejs/tsc Since @petkaantonov has been too busy to do anything with #1650 and it's at a point where it needs rebasing anyway, I suggest we consider this for 3.x.

[yorkie]

If @petkaantonov and one or more of TSC agreed with, I would be so happy to working on #1650 from July so that we are able to work on this as well. This 2 has been suspended too long IMHO :)

[chrisdickinson]

@Qard Hold up on merging this still, this will break a lot of downstream code.