url: update the processing in the scheme state.

[watilde]

Fix #11785. I've updated the processing in the scheme state to follow the specification and synchronized url-setter-tests with upstream.

• Related lines in the spec:
  https://github.com/whatwg/url/blob/a8ed8d2aefeec1e742490d08844c3d91a87e77fb/url.bs#L1493-L1508

While I was testing added tests, I noticed that the current state machine of Node.js doesn't parse URL correctly for pathname. For example:

var url = new URL('ssh://example.net')
url.pathname
// expected: //example.net
// actual: '/'

So I got rid of the following test for this time.

{
  "href": "ssh://example.net",
  "new_value": "file",
  "expected": {
    "href": "ssh://example.net",
    "protocol": "ssh:"
  }
},

I will work on it later.

Checklist

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

Affected core subsystem(s)

url, test

[watilde]

cc @nodejs/url

[TimothyGu]

The returning is handled in the JS layer. You might want to update that instead.

[watilde]

Oh that's nice. I will update the commit then!

[watilde]

@TimothyGu I've updated the commits to handling it in JS layer.

Well, I'm not sure those steps should be handled in the scheme state or not, but at least the spec expects it as scheme state checking. What are your thoughts on?

[TimothyGu]

port can only be >= 0 or undefined, since the C++ layer sets port to undefined if it is -1, and you should probably check ctx.username and ctx.password instead of username and password since IIRC the C++ layer doesn't pass those into the callback if it's unneeded.

[jasnell]

Good catch. +1

[watilde]

That's right! I will update it. Thanks 😺

[watilde]

Ok, I've updated.

[watilde]

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

[TimothyGu]

LGTM other than the nit.

[TimothyGu]

nit: stylistically it's more common to write it as:

if (protocol === 'file:' &&
    (ctx.username || ctx.password || ctx.port !== undefined)) {

[watilde]

new CI() https://ci.nodejs.org/job/node-test-pull-request/6912/

[joyeecheung]

If there are test cases that don't pass at the moment, you can change url-setter-tests.json to a js file with module.exports, then put those test cases in that file and comment them out, like what https://github.com/nodejs/node/blob/master/test/fixtures/url-tests.js does

[watilde]

@joyeecheung that's nice! I will sync the setter url fixture in my next patch, will rename the file at the same time then :)

[watilde]

Landed in 60c8a35 and cab58fe.