Skip to content

http,https: avoid instanceof for WHATWG URL#12983

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.

Sign up for GitHub

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

Merged
mscdex merged 1 commit into from
May 22, 2017
Merged

http,https: avoid instanceof for WHATWG URL #12983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/_http_client.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,7 +34,7 @@ const debug = common.debug;
const OutgoingMessage = require('_http_outgoing').OutgoingMessage;
const Agent = require('_http_agent');
const Buffer = require('buffer').Buffer;
const urlToOptions= require('internal/url').urlToOptions;
const {urlToOptions, searchParamsSymbol } = require('internal/url');
const outHeadersKey = require('internal/http').outHeadersKey;
const nextTick = require('internal/process/next_tick').nextTick;

Expand DownExpand Up@@ -82,7 +82,9 @@ function ClientRequest(options, cb){
if (!options.hostname){
throw new Error('Unable to determine the domain name');
}
} else if (options instanceof url.URL){
} else if (options && options[searchParamsSymbol] &&
options[searchParamsSymbol][searchParamsSymbol]){
Copy link
Member

@joyeecheungjoyeecheungMay 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now wondering if this way of checking URLs is a bit hard to understand for someone who don't know the implementation details of URL...

Copy link
ContributorAuthor

@mscdexmscdexMay 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not a whole lot that can be done about that. Using a helper function as previously noted incurs a measurable performance hit.

Copy link
Member

@joyeecheungjoyeecheungMay 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex Maybe a comment would help?

Copy link
ContributorAuthor

@mscdexmscdexMay 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// url.URL instance
options = urlToOptions(options);
} else{
options = util._extend({}, options);
Expand Down
6 changes: 4 additions & 2 deletions lib/https.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -29,7 +29,7 @@ const http = require('http');
const util = require('util');
const inherits = util.inherits;
const debug = util.debuglog('https');
const urlToOptions= require('internal/url').urlToOptions;
const {urlToOptions, searchParamsSymbol } = require('internal/url');

function Server(opts, requestListener){
if (!(this instanceof Server)) return new Server(opts, requestListener);
Expand DownExpand Up@@ -221,7 +221,9 @@ exports.request = function request(options, cb){
if (!options.hostname){
throw new Error('Unable to determine the domain name');
}
} else if (options instanceof url.URL){
} else if (options && options[searchParamsSymbol] &&
options[searchParamsSymbol][searchParamsSymbol]){
Copy link
Member

@jasnelljasnellMay 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a require('internal/url').isURL() utility function be worthwhile to make this more convenient? Or would that kill any perf gain? I imagine wanting to use similar checks elsewhere.

Copy link
ContributorAuthor

@mscdexmscdexMay 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here.

Copy link
Member

@jasnelljasnellMay 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! I thought the question sounded familiar as I was typing it ;-)

// url.URL instance
options = urlToOptions(options);
} else{
options = util._extend({}, options);
Expand Down