url: update WHATWG URL parser to latest spec #12523

TimothyGu · 2017-04-19T22:09:11Z

The following (non-editorial) spec changes are implemented in this PR:

Add opaque hosts (Add opaque hosts whatwg/url#185)
File state did not correctly deal with lack of base URL (File state did not correctly deal with lack of base URL whatwg/url#225)
Cleanup API for file and non-special URLs (Cleanup API for file and non-special URLs whatwg/url#224)
Allow % and IPv6 addresses in non-special URL hosts (Fix: allow percent encoded code points in the non-special URLs host whatwg/url#218)
Use specific names for percent-encode sets (Editorial: use specific names for encode sets whatwg/url#243)
Add empty host concept for file and non-special URLs (Use empty host for file URL's whatwg/url#260)
Clarify IPv6 serializer (Clarify IPv6 serializer whatwg/url#268)

I had to squash the four spec updates into one commit, since some of them overlap with one another, making it difficult to separate atomic changes.

This also enables all tests in url-tests.js, which means that our implementation will be fully spec-compliant after this PR. 🎉

Fixes: #10608
Fixes: #10634

Checklist

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

Affected core subsystem(s)

url

TimothyGu · 2017-04-19T22:13:34Z

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

jasnell · 2017-04-19T22:19:55Z

FYI: Once this lands, and we can verify that we're fully spec compliant, I want to open a PR that moves the WHATWG URL out of Experimental status.

jasnell

LGTM with a nit

jasnell · 2017-04-19T22:21:18Z

lib/internal/url.js

@@ -8,6 +8,8 @@ const {
 const binding = process.binding('url');
 const context = Symbol('context');
 const cannotBeBase = Symbol('cannot-be-base');
+const cannotHaveUsernamePasswordPort =
+    Symbol('cannot have a username/password/port');


Perhaps Symbol('cannot-have-username-password-port') to remain consistent with the other symbols?

mscdex · 2017-04-19T22:33:50Z

lib/internal/url.js

+  // https://url.spec.whatwg.org/#cannot-have-a-username-password-port
+  get [cannotHaveUsernamePasswordPort]() {
+    const { host, scheme } = this[context];
+    return ((host == null || host === '') ||


Perhaps host.length === 0 might be better/faster?

The spec says "if its host is null or the empty string" so it'll be clearer this way, and no, it's not faster in my tests.

TimothyGu · 2017-04-19T22:34:49Z

I want to open a PR that moves the WHATWG URL out of Experimental status.

Sounds good to me. Will this for v8.x?

jasnell · 2017-04-19T22:35:32Z

Hopefully we can do it in time for 8.0.0

TimothyGu · 2017-04-20T01:50:42Z

@jasnell nit fixed.

watilde · 2017-04-20T09:18:46Z

Great work! Can you also update test/fixture/url-setter-tests.js as well?

TimothyGu · 2017-04-20T18:54:54Z

@watilde, done. That actually made me notice a few bugs that were in the initial implementation, thanks! 👏

@jasnell I'd like you to take a look at this again, especially 3f53f8038fa676618db6f8169f93ba53aeaebc0c which changes how the JS-C++ layer works right now.

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

jasnell · 2017-04-20T18:56:25Z

Ok, I'll take a look in detail shortly

- Update to spec - Add opaque hosts - File state did not correctly deal with lack of base URL - Cleanup API for file and non-special URLs - Allow % and IPv6 addresses in non-special URL hosts - Use specific names for percent-encode sets - Add empty host concept for file and non-special URLs - Clarify IPv6 serializer - Fix existing mistakes - Add missing ':' to forbidden host code point list. - Correct IPv4 parser empty label behavior - Maintain type equivalence in URLContext with spec - scheme, username, and password should always be strings - host, port, query, and fragment may be strings or null - Align scheme state more closely with the spec - Make sure the `special` variable is always synced with URL_FLAG_SPECIAL. PR-URL: nodejs#12523 Fixes: nodejs#10608 Fixes: nodejs#10634 Refs: whatwg/url#185 Refs: whatwg/url#225 Refs: whatwg/url#224 Refs: whatwg/url#218 Refs: whatwg/url#243 Refs: whatwg/url#260 Refs: whatwg/url#268 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

TimothyGu · 2017-04-23T18:27:28Z

Also implemented whatwg/url#260.

One more CI: https://ci.nodejs.org/job/node-test-pull-request/7622/

TimothyGu · 2017-04-24T23:36:41Z

Landed in b2870a4.

- Update to spec - Add opaque hosts - File state did not correctly deal with lack of base URL - Cleanup API for file and non-special URLs - Allow % and IPv6 addresses in non-special URL hosts - Use specific names for percent-encode sets - Add empty host concept for file and non-special URLs - Clarify IPv6 serializer - Fix existing mistakes - Add missing ':' to forbidden host code point list. - Correct IPv4 parser empty label behavior - Maintain type equivalence in URLContext with spec - scheme, username, and password should always be strings - host, port, query, and fragment may be strings or null - Align scheme state more closely with the spec - Make sure the `special` variable is always synced with URL_FLAG_SPECIAL. PR-URL: #12523 Fixes: #10608 Fixes: #10634 Refs: whatwg/url#185 Refs: whatwg/url#225 Refs: whatwg/url#224 Refs: whatwg/url#218 Refs: whatwg/url#243 Refs: whatwg/url#260 Refs: whatwg/url#268 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 19, 2017

jasnell approved these changes Apr 19, 2017

View reviewed changes

mscdex reviewed Apr 19, 2017

View reviewed changes

watilde mentioned this pull request Apr 20, 2017

url: track whatwg url issue #185 (non-special URL host) #10634

Closed

TimothyGu force-pushed the url-perc-nonspecial branch 2 times, most recently from 6257d18 to 750f452 Compare April 20, 2017 18:45

watilde approved these changes Apr 20, 2017

View reviewed changes

joyeecheung approved these changes Apr 21, 2017

View reviewed changes

jasnell approved these changes Apr 21, 2017

View reviewed changes

TimothyGu mentioned this pull request Apr 22, 2017

url: ensure host setter matches parse for file url #10615

Closed

3 tasks

TimothyGu force-pushed the url-perc-nonspecial branch from 750f452 to 7c0704d Compare April 23, 2017 18:05

TimothyGu force-pushed the url-perc-nonspecial branch from 7c0704d to e0530f1 Compare April 23, 2017 18:14

watilde approved these changes Apr 24, 2017

View reviewed changes

TimothyGu closed this Apr 24, 2017

TimothyGu deleted the url-perc-nonspecial branch April 24, 2017 23:36

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: update WHATWG URL parser to latest spec #12523

url: update WHATWG URL parser to latest spec #12523

TimothyGu commented Apr 19, 2017 •

edited

Loading

TimothyGu commented Apr 19, 2017

jasnell commented Apr 19, 2017

jasnell left a comment

jasnell Apr 19, 2017

mscdex Apr 19, 2017

TimothyGu Apr 20, 2017 •

edited

Loading

TimothyGu commented Apr 19, 2017

jasnell commented Apr 19, 2017

TimothyGu commented Apr 20, 2017

watilde commented Apr 20, 2017

TimothyGu commented Apr 20, 2017 •

edited

Loading

jasnell commented Apr 20, 2017

TimothyGu commented Apr 23, 2017

TimothyGu commented Apr 24, 2017

url: update WHATWG URL parser to latest spec #12523

url: update WHATWG URL parser to latest spec #12523

Conversation

TimothyGu commented Apr 19, 2017 • edited Loading

Checklist

Affected core subsystem(s)

TimothyGu commented Apr 19, 2017

jasnell commented Apr 19, 2017

jasnell left a comment

Choose a reason for hiding this comment

jasnell Apr 19, 2017

Choose a reason for hiding this comment

mscdex Apr 19, 2017

Choose a reason for hiding this comment

TimothyGu Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

TimothyGu commented Apr 19, 2017

jasnell commented Apr 19, 2017

TimothyGu commented Apr 20, 2017

watilde commented Apr 20, 2017

TimothyGu commented Apr 20, 2017 • edited Loading

jasnell commented Apr 20, 2017

TimothyGu commented Apr 23, 2017

TimothyGu commented Apr 24, 2017

TimothyGu commented Apr 19, 2017 •

edited

Loading

TimothyGu Apr 20, 2017 •

edited

Loading

TimothyGu commented Apr 20, 2017 •

edited

Loading