url: improve WHATWG URL inspection by TimothyGu · Pull Request #12253 · nodejs/node

TimothyGu · 2017-04-06T04:57:32Z

Show all available properties (including searchParams!)
Colors!
Respect depth
Throw an error if this is not an actual URL
Inspect [context] if showHidden is true

Before:

After:

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

mscdex · 2017-04-06T07:25:20Z

lib/internal/url.js

+      port: this.port,
+      pathname: this.pathname,
+      search: this.search,
+      searchParams: this.searchParams,


Shouldn't this be this[searchParams] since searchParams is a Symbol and not a regular property name?

Additionally, I think it's very misleading to be representing the searchParams as a regular property when it cannot be accessed as such.

url.searchParams is a property one can access: https://nodejs.org/api/url.html#url_url_searchparams

Ah ok, I missed that.

mscdex · 2017-04-06T07:27:11Z

lib/internal/url.js

-    return ret;
+
+    return util.inspect(obj, opts).replace(/^URLTemp/,
+                                           () => this.constructor.name);


Couldn't this be simplified to just be 'URL'?

I wanted to allow subclassing. For example, with this PR and

class MyURL extends URL {}

inspecting new MyURL('...') will show MyURL instead URL.

mscdex · 2017-04-06T07:29:00Z

lib/internal/url.js

+      protocol: this.protocol,
+      username: this.username,
+      password: (opts.showHidden || ctx.password == null) ?
+        this.password : '--------',


This might be confusing/unexpected behavior for some... Why hide it at all? There does not seem to be a precedent for this AFAICT. For example the browser seems to show the password value every time when inspecting it.

/cc @jasnell

This was me just being paranoid ;-) ... I'm fine with showing the password here.

mscdex · 2017-04-06T07:35:35Z

lib/internal/url.js

-    ret += '}';
-    return ret;
+
+    return util.inspect(obj, opts).replace(/^URLTemp/,


This replace() can be avoided by simply doing var URL = function() {}; at the top of the function here and then simply using new URL() instead of new URLTemp().

mscdex · 2017-04-06T07:36:34Z

lib/internal/url.js

+    if (typeof depth === 'number' && depth < 0)
+      return opts.stylize('[Object]', 'special');
+
+    const obj = Object.assign(new URLTemp(), {


I think we should avoid Object.assign(), especially when we're already using a custom/"fake" constructor which could easily assign its own appropriate values in its constructor (e.g. just pass this to the constructor).

mscdex · 2017-04-07T13:18:52Z

Did you benchmark the hasOwnProperty() loop vs. the replace()? I'm wondering if the replace() would be faster?

jasnell · 2017-04-11T10:02:27Z

test/parallel/test-whatwg-url-inspect.js

+  port: '8080',
+  pathname: '/path/name/',
+  search: '?que=ry',
+  searchParams: URLSearchParams { 'que' => 'ry' },


Can't really say that I'm a fan of the use of => here instead of a :.

That is to be consistent with the inspection output of Map, plus it's weird to write

{ query: 'value1', query: 'value2' }

with object literal syntax when it doesn't really work.

Well, multiple values could be shown together as an array?

{ query: [ 'value1', 'value2' ] }

@mscdex that'd remove the order of queries.

addaleax · 2017-04-14T22:28:33Z

Landed in 2841f47

PR-URL: #12253 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

url: improve WHATWG URL inspection

913a6d0

nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 6, 2017

mscdex reviewed Apr 6, 2017

View reviewed changes

Avoid using hacky temporary constructor

0f48f3e

jasnell reviewed Apr 11, 2017

View reviewed changes

jasnell approved these changes Apr 12, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

addaleax approved these changes Apr 14, 2017

View reviewed changes

addaleax closed this Apr 14, 2017

addaleax pushed a commit that referenced this pull request Apr 14, 2017

url: improve WHATWG URL inspection

2841f47

PR-URL: #12253 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

TimothyGu deleted the url-inspect branch April 14, 2017 22:31

TimothyGu mentioned this pull request Apr 19, 2017

(v7.x backport) url: updates to the WHATWG URL parser #12507

Merged

4 tasks

TimothyGu added the backport-requested-v7.x label Apr 19, 2017

TimothyGu added backported-to-v7.x and removed backport-requested-v7.x labels Apr 27, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

Uh oh!

Conversation

TimothyGu commented Apr 6, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mscdex Apr 6, 2017 •

edited

Loading

mscdex Apr 6, 2017 •

edited

Loading