feat: change code to use url.URL instead of url.parse

[yuxizhe]

close #1254

Checklist

• Implement code

[Qard]

jenkins run the tests please

[Qard]

Marked as breaking change as the URL class does not exist yet in Node.js 6 so we have to finish dropping v6 first.

[watson]

Great work!

I just left a few review comments and then it should be good to go.

Another recommendation that I would make would be to move the logic of checking if url.URL is available into a utility function which you can just call instead, so:

url.URL ? new url.URL(urlStr, 'relative:///') : url.parse(urlStr)

Would be turned into

parseUrl(urlStr)

And parseUrl could then just be a function on lib/parsers.js:

exports.parseUrl = function (urlStr) {
  return url.URL ? new url.URL(urlStr, 'relative:///') : url.parse(urlStr)
}

[yuxizhe]

@watson All fixed, Thanks for your advice 👍

[Qard]

jenkins run the tests please

[Qard]

jenkins run the tests please

[Qard]

So with this using both url.Url and url.parse(...), are we targeting landing this in 2.x as a not a breaking change? If so, it appears to be failing in Node.js 6.x and we should figure out why. If we want to just do this as a breaking change in 3.x, it should be fine as-is.

[watson]

I think it's easiest to target 2.x now and then we can just remove the url.parse fallback once we're in 3.x. That way we get the best of both worlds with almost no effort

[watson]

I'll just look into the failing Node.js 6 test...

[yuxizhe]

I'll just look into the failing Node.js 6 test...

Very strange. It passed travis-ci node 6 tests, but failed in apm-ci node 6. I can't reproduce that error on my computer with node 6.x.

[watson]

I've seen this specific test failure on Jenkins a few times in other PRs lately. So I think the Node.js 6 tests are just flaky when running on Jenkins. I'll go ahead and run the tests one more time to see if it goes away.

[watson]

jenkins run the tests please

[watson]

This time around the

[watson]

Ups, I pressed the wrong button 🤦‍♂

Was about to write that this time around the Node.js 6 tests passed just fine 😅

[watson]

I've created another issue with the Node.js 6 failures on Jenkins: #1350

[watson]

Thanks a lot for helping us fix this one @yuxizhe 💯 It'll be part of the next release 😃

[yuxizhe]

Thanks a lot for helping us fix this one @yuxizhe 💯 It'll be part of the next release 😃

My pleasure : )