Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: improve WHATWG URL inspection #12253

Closed
wants to merge 2 commits into from

Conversation

@TimothyGu
Copy link
Member

commented Apr 6, 2017

  • 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:

screenshot from 2017-04-05 21 53 17

After:

screenshot from 2017-04-04 20 21 43

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

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

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 6, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Author Member

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

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 6, 2017

Contributor

Ah ok, I missed that.

return ret;

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

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Author Member

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

class MyURL extends URL {}

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

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

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 6, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 6, 2017

Author Member

/cc @jasnell

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 11, 2017

Member

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

ret += '}';
return ret;

return util.inspect(obj, opts).replace(/^URLTemp/,

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 6, 2017

Contributor

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().

if (typeof depth === 'number' && depth < 0)
return opts.stylize('[Object]', 'special');

const obj = Object.assign(new URLTemp(), {

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 6, 2017

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

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

@joyeecheung joyeecheung added this to Spec Irrelavent / De-facto in WHATWG URL implementation Apr 11, 2017

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

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 11, 2017

Member

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

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 11, 2017

Author Member

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.

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 12, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 12, 2017

Author Member

@mscdex that'd remove the order of queries.

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

Landed in 2841f47

@addaleax addaleax closed this Apr 14, 2017

addaleax added a commit that referenced this pull request Apr 14, 2017
url: improve WHATWG URL inspection
PR-URL: #12253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@TimothyGu TimothyGu deleted the TimothyGu:url-inspect branch Apr 14, 2017

@watilde watilde moved this from Spec Irrelavent / De-facto to Done in WHATWG URL implementation Apr 17, 2017

@TimothyGu TimothyGu referenced this pull request Apr 19, 2017
4 of 4 tasks complete
@jasnell jasnell referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.