url: make path in resolveObject the same as parse

[watilde]

url.resolveObject should not add / automatically to have a compatibility with url.parse if provided value doesn't have any path.

I've also added a test for it. It increases the coverage of url.js,

• https://coverage.nodejs.org/coverage-e4e7cd5bd202084b/root/url.js.html

and it will cover these lines:

• node/lib/url.js

  Lines 830 to 842 in e4e7cd5

  	if (!srcPath.length) {
  	// no path at all. easy.
  	// we've already handled the other stuff above.
  	result.pathname = null;
  	//to support http.request
  	if (result.search) {
  	result.path = '/' + result.search;
  	} else {
  	result.path = null;
  	}
  	result.href = result.format();
  	return result;
  	}

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

url, test

[jasnell]

/cc @mscdex

[mscdex]

Added semver-major because of the change in output. I don't have an opinion way or the other on the actual change.

[jasnell]

@watilde ... can you rebase this?

[watilde]

@jasnell done!

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/6980/

[watilde]

Does anyone have an opinion on this change? @nodejs/collaborators

[Trott]

Maybe @TimothyGu or @joyeecheung might have an opinion?

[benjamingr]

I'm not convinced the added value justifies breaking apps in the wild. In the very list I'd like to see a citgm run here.

[jasnell]

ping @mscdex

[joyeecheung]

+1 on a CITGM run...so:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/680/
EDIT: oops, I filled in a wrong refspec...here is the correct one: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/681/

(The citgm-smoker is all-red now so I am not sure if we need to launch another one against master for comparison?)

[addaleax]

(The citgm-smoker is all-red now so I am not sure if we need to launch another one against master for comparison?)

None of the failures in the CITGM run should be related to this PR, they also show up in the other runs.

[watilde]

ci again: https://ci.nodejs.org/job/node-test-pull-request/7736/

[watilde]

We still have a chance to put this into v8.x. The original behaviour could be an unexpectable action and the only way to use it is passing non-path values like this: ['#hash', '?query', '?query#hash'].

[watilde]

Now I think this is a very small thing, and we can look back at here when it's needed. I'm going to close this.

Thanks for your all comments anyway :)