url: extend url.format to support WHATWG URL #10857

jasnell · 2017-01-17T17:11:47Z

Removes the non-standard options on WHATWG URL toString
and extends the existing url.format() API to support
customizable serialization of the WHATWG URL object.

This does not yet include the documentation updates
because the documentation for the new WHATWG URL object
has not yet landed.

Example:

const url = require('url');
const URL = url.URL;
const myURL = new URL('http://example.org/?a=b#c');
const str = url.format(myURL, {fragment: false, search: false});
console.log(str);
  // Prints: http://example.org/

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, whatwg-url

/cc @watilde @nodejs/url

joyeecheung · 2017-01-17T17:42:59Z

lib/internal/url.js

@@ -263,6 +256,48 @@ class URL {
    ret += '}';
    return ret;
  }
+
+  [kFormat](options) {


Symbols added this way are enumerable(can be verified via Object.getOwnPropertyDescriptor(url, kFormat)), I believe test-whatwg-url-properties doesn't catch this because for-in only iterates over string keys, but since Reflect.enumerate is withdrawn in ES2016 I can't think of a way to make that test more complete...anyways I think we should useObject.defineProperty here to make it non-enumerable, or just move this function out and use .call to call it properly?

Using defineProperty is fine.

Putting the symbols here at all (enumerable or not) creates an observable difference from the spec via Object.getOwnPropertySymbols. But I guess there are several of those already (special/cannotBeABase) so IMO it's probably not worth changing...

mscdex · 2017-01-17T18:06:56Z

How much of a performance impact does this have on non-whatwg url usage?

watilde · 2017-01-17T19:02:03Z

It may be another topic, but from the viewpoint of making the URL object behave the same as the standard, is URL.inspect() OK even when it can be called? I think it could be replaced with util.inspect when it needs.

Here is an example:

node/test/parallel/test-whatwg-url-parsing.js

Lines 147 to 149 in d86ff5f

    
           const res = url.inspect(null, { 
        
             showHidden: showHidden 
        
           });

jasnell · 2017-01-17T19:09:04Z

@mscdex ... the performance hit incurred by the addition of the new instanceof URL check is, unfortunately, non-trivial. I'll play around with it to see how I can reduce the cost.

jasnell · 2017-01-17T19:09:54Z

@watilde ... we now have the option of using a symbol for the inspect function. It would make sense to switch to using that instead of the inspect() function itself.

jasnell · 2017-01-17T19:23:19Z

@mscdex @joyeecheung ... updated. modified the changes in url.format a bit to reduce the cost of the additional check.

addaleax · 2017-01-18T11:03:09Z

lib/internal/url.js

-          ret += this.host;
-        }
+        ret += options.unicode ?
+          domainToUnicode(this.host) : this.host;


Is the unicode option name from the spec, from compatibility with the old url, or is it up for debate? I don’t think it’s conveying the semantics very well (like, I have to look at the tests in this PR to see which behaviour maps to which value). How does something like decodeIDNA: true/false sound to you?

(Strange, my response ended up both in my own review and here, but when I deleted that one in my review this one disappeared too)..

I think options is neither in the spec nor in the old url.format. For me the meaning of unicode is pretty straightforword(maybe it's just me though). The two serialization alternatives are named Domain to ASCII and Domain to Unicode in the spec.

The unicode flag is consistent with the URL standard, yes (see: https://url.spec.whatwg.org/#host-parsing). To be fair, however, that is on parse side and this is on the serialization, but there is precedent at least.

The unicode flag is consistent with the URL standard, yes (see: https://url.spec.whatwg.org/#host-parsing). To be fair, however, that is on parse side and this is on the serialization, but there is precedent at least.

Eh, if it’s in the standard, even just on the parse side, it’s probably best to leave it like that. Thanks for pointing it out.

For me the meaning of unicode is pretty straightforword(maybe it's just me though).

Well, if it’s working and people know what the option describes, that’s probably just fine then. (At least in my head, unicode: true sounds like it could both mean enabling a) decoding of the hostname from punycode to Unicode or b) encoding of the hostname from Unicode to punycode)

I will make sure that the relevant documentation appropriately covers the option and makes its function clear

joyeecheung · 2017-01-18T12:21:35Z

lib/internal/url.js

+      if (typeof options !== 'object')
+        throw new TypeError('options must be an object');
+      options.fragment = options.fragment !== undefined ?
+        Boolean(options.fragment) : true;


Why use Boolean() instead of !! here?

Personal preference with regards to code clarity

joyeecheung · 2017-01-18T12:22:32Z

lib/internal/url.js

@@ -286,38 +283,43 @@ Object.defineProperties(URL.prototype, {
        ret += '//';
        const has_username = typeof ctx.username === 'string';
        const has_password = typeof ctx.password === 'string' &&
-                             ctx.password !== '';
-        if (has_username || has_password) {
+                              ctx.password !== '';


nit: extra whitespace here

This still hasn't been fixed.

joyeecheung · 2017-01-18T12:34:24Z

lib/internal/url.js

  href: {
    enumerable: true,
    configurable: true,
    get() {
-      return this.toString();
+      return this[kFormat]({});


I think keep it as this.toString() is closer to how the spec defines href as the stringifier.

The result is the same. Using this[kFormat]({}) here avoids an unnecessary property access.

TimothyGu · 2017-01-20T02:33:40Z

lib/internal/url.js

-    configurable: true,
+  [kFormat]: {
+    enumerable: false,
+    configurable: false,


These two lines are not needed as they are the default. However, the trend in Web APIs seems to be make symbol properties configurable and writable, so I'd recommend sticking to that

Yes, I know they are the default. I prefer to be explicit. Also, because this is an internal only method, I prefer to keep these values

joyeecheung · 2017-01-20T08:02:15Z

LGTM except it needs rebase.

jasnell · 2017-01-23T18:00:22Z

@mscdex ... any further thoughts on this?

mscdex · 2017-01-23T21:12:26Z

@jasnell Performance-wise the lib/url.js changes LGTM.

Removes the non-standard options on WHATWG URL toString and extends the existing url.format() API to support customizable serialization of the WHATWG URL object. This does not yet include the documentation updates because the documentation for the new WHATWG URL object has not yet landed. Example: ```js const url = require('url'); const URL = url.URL; const myURL = new URL('http://example.org/?a=b#c'); const str = url.format(myURL, {fragment: false, search: false}); console.log(str); // Prints: http://example.org/ ```

Removes the non-standard options on WHATWG URL toString and extends the existing url.format() API to support customizable serialization of the WHATWG URL object. This does not yet include the documentation updates because the documentation for the new WHATWG URL object has not yet landed. Example: ```js const url = require('url'); const URL = url.URL; const myURL = new URL('http://example.org/?a=b#c'); const str = url.format(myURL, {fragment: false, search: false}); console.log(str); // Prints: http://example.org/ ``` PR-URL: #10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

PR-URL: #10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2017-02-02T20:38:35Z

Landed in c5e9654...0d9ea4f

Removes the non-standard options on WHATWG URL toString and extends the existing url.format() API to support customizable serialization of the WHATWG URL object. This does not yet include the documentation updates because the documentation for the new WHATWG URL object has not yet landed. Example: ```js const url = require('url'); const URL = url.URL; const myURL = new URL('http://example.org/?a=b#c'); const str = url.format(myURL, {fragment: false, search: false}); console.log(str); // Prints: http://example.org/ ``` PR-URL: #10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

PR-URL: #10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Removes the non-standard options on WHATWG URL toString and extends the existing url.format() API to support customizable serialization of the WHATWG URL object. This does not yet include the documentation updates because the documentation for the new WHATWG URL object has not yet landed. Example: ```js const url = require('url'); const URL = url.URL; const myURL = new URL('http://example.org/?a=b#c'); const str = url.format(myURL, {fragment: false, search: false}); console.log(str); // Prints: http://example.org/ ``` PR-URL: nodejs#10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](#11094) * add node-inspect 1.10.2 (Jan Krems) [#10187](#10187) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](#11149) * fs: allow WHATWG URL and file: URLs as paths (James M Snell) [#10739](#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](#10857) PR-URL: #11185

Backport-of: nodejs#10857

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [nodejs#11029](nodejs#11029) * upgrade libuv to 1.11.0 (cjihrig) [nodejs#11094](nodejs#11094) * add node-inspect 1.10.4 (Jan Krems) [nodejs#10187](nodejs#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [nodejs#10980](nodejs#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [nodejs#10187](nodejs#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [nodejs#9469](nodejs#9469) * inspector: add --inspect-brk (Josh Gavant) [nodejs#11149](nodejs#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [nodejs#10739](nodejs#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [nodejs#11129](nodejs#11129) * url: extend url.format to support WHATWG URL (James M Snell) [nodejs#10857](nodejs#10857) PR-URL: nodejs#11185

Backport-of: #10857

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [nodejs#11029](nodejs#11029) * upgrade libuv to 1.11.0 (cjihrig) [nodejs#11094](nodejs#11094) * add node-inspect 1.10.4 (Jan Krems) [nodejs#10187](nodejs#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [nodejs#10980](nodejs#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [nodejs#10187](nodejs#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [nodejs#9469](nodejs#9469) * inspector: add --inspect-brk (Josh Gavant) [nodejs#11149](nodejs#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [nodejs#10739](nodejs#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [nodejs#11129](nodejs#11129) * url: extend url.format to support WHATWG URL (James M Snell) [nodejs#10857](nodejs#10857) PR-URL: nodejs#11185

Removes the non-standard options on WHATWG URL toString and extends the existing url.format() API to support customizable serialization of the WHATWG URL object. This does not yet include the documentation updates because the documentation for the new WHATWG URL object has not yet landed. Example: ```js const url = require('url'); const URL = url.URL; const myURL = new URL('http://example.org/?a=b#c'); const str = url.format(myURL, {fragment: false, search: false}); console.log(str); // Prints: http://example.org/ ``` PR-URL: nodejs#10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

PR-URL: nodejs#10857 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](nodejs/node#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](nodejs/node#11094) * add node-inspect 1.10.4 (Jan Krems) [#10187](nodejs/node#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [#10980](nodejs/node#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](nodejs/node#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](nodejs/node#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](nodejs/node#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [#10739](nodejs/node#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](nodejs/node#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](nodejs/node#10857) PR-URL: nodejs/node#11185 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 17, 2017

nodejs-github-bot added dont-land-on-v4.x url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 17, 2017

jasnell removed the dont-land-on-v7.x label Jan 17, 2017

joyeecheung reviewed Jan 17, 2017

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Jan 17, 2017

addaleax reviewed Jan 18, 2017

View reviewed changes

joyeecheung reviewed Jan 18, 2017

View reviewed changes

TimothyGu mentioned this pull request Jan 20, 2017

url: use symbol properties correctly in URL class #10906

Closed

3 tasks

TimothyGu reviewed Jan 20, 2017

View reviewed changes

joyeecheung approved these changes Jan 20, 2017

View reviewed changes

addaleax approved these changes Jan 21, 2017

View reviewed changes

joyeecheung mentioned this pull request Jan 22, 2017

Remove nonstandard WHATWG URL url.toString(true) #10750

Closed

TimothyGu removed the dont-land-on-v7.x label Jan 22, 2017

jasnell force-pushed the fixup-url-tostring branch from f98e923 to 88eee79 Compare January 23, 2017 18:03

nodejs-github-bot added the dont-land-on-v7.x label Jan 23, 2017

jasnell removed the dont-land-on-v7.x label Jan 23, 2017

jasnell added 2 commits February 2, 2017 12:33

doc: add documentation for url.format(URL[, options]);

d1d7b02

jasnell force-pushed the fixup-url-tostring branch from 25c0c7c to d1d7b02 Compare February 2, 2017 20:35

jasnell closed this Feb 2, 2017

italoacasas mentioned this pull request Feb 5, 2017

Proposal - 7.6.0 (Current) #11185

Merged

TimothyGu pushed a commit to TimothyGu/node that referenced this pull request Feb 18, 2017

doc: add documentation for url.format(URL[, options]);

a454801

Backport-of: nodejs#10857

TimothyGu mentioned this pull request Feb 18, 2017

(v7.x backport) URL-related changes #11457

Closed

4 tasks

italoacasas added the dont-land-on-v7.x label Feb 20, 2017

italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017

doc: add documentation for url.format(URL[, options]);

dea3bdb

Backport-of: nodejs#10857

italoacasas pushed a commit that referenced this pull request Feb 22, 2017

doc: add documentation for url.format(URL[, options]);

5a2db15

Backport-of: #10857

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: extend url.format to support WHATWG URL #10857

url: extend url.format to support WHATWG URL #10857

jasnell commented Jan 17, 2017

joyeecheung Jan 17, 2017

jasnell Jan 17, 2017

domenic Jan 17, 2017

mscdex commented Jan 17, 2017 •

edited

Loading

watilde commented Jan 17, 2017

jasnell commented Jan 17, 2017

jasnell commented Jan 17, 2017

jasnell commented Jan 17, 2017

addaleax Jan 18, 2017

joyeecheung Jan 18, 2017

jasnell Jan 18, 2017

addaleax Jan 18, 2017

jasnell Jan 18, 2017

joyeecheung Jan 18, 2017

jasnell Jan 18, 2017

joyeecheung Jan 18, 2017

TimothyGu Jan 24, 2017

joyeecheung Jan 18, 2017

jasnell Jan 18, 2017 •

edited

Loading

TimothyGu Jan 20, 2017

jasnell Jan 20, 2017

joyeecheung commented Jan 20, 2017

jasnell commented Jan 23, 2017

mscdex commented Jan 23, 2017

jasnell commented Feb 2, 2017

url: extend url.format to support WHATWG URL #10857

url: extend url.format to support WHATWG URL #10857

Conversation

jasnell commented Jan 17, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 17, 2017 • edited Loading

watilde commented Jan 17, 2017

jasnell commented Jan 17, 2017

jasnell commented Jan 17, 2017

jasnell commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Jan 20, 2017

jasnell commented Jan 23, 2017

mscdex commented Jan 23, 2017

jasnell commented Feb 2, 2017

mscdex commented Jan 17, 2017 •

edited

Loading

jasnell Jan 18, 2017 •

edited

Loading