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

tls: remove util and calls to util.format #3456

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@MylesBorins
Copy link
Member

commented Oct 20, 2015

Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2015

/cc @jasnell

@bnoordhuis

View changes

lib/tls.js Outdated
host,
cert.subjectaltname);
reason =
`Host: ${host} is not in the cert's altnames: ${cert.subjectaltname}`;

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 20, 2015

Member

Style nit: line continuations should indent by four spaces. Didn't the linter complain?

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 20, 2015

Author Member

linter did not complain. Fixing this right now.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 20, 2015

Author Member

So funny enough, if this line is indented an extra two characters it is over 80 :(

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 20, 2015

Member

Hah, okay. You can break it in two, i.e.:

reason = `Host: ${host} is not in the cert's altnames: ` +
         `${cert.subjectaltname}`;

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 20, 2015

Author Member

fixed

@@ -189,8 +183,7 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {

if (!valid) {
var err = new Error(
util.format('Hostname/IP doesn\'t match certificate\'s altnames: %j',
reason));
`Hostname/IP doesn't match certificate's altnames: "${reason}"`);

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Oct 20, 2015

Member

Why use quotes around the code it at the end here?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 20, 2015

Member

It's because of the %j (JSON.stringify) it replaces. reason is always a string so putting it in quotes looks right to me.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 20, 2015

Author Member

JSON.stringify does offer protection for circular references... I don't think reason would cause that, but would appreciate a +1 that we don't have to worry about that.

As an aside, how do backticks handle circular refs?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 20, 2015

Member

It's always a string, right? It can't have circular references.

As an aside, how do backticks handle circular refs?

They don't, interpolated values are simply stringified with .toString().

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 20, 2015

Author Member

I was assuming it was always a string but didn't dig through the code (best to dummy check :D)

This comment has been minimized.

Copy link
@jasnell

jasnell Oct 20, 2015

Member

Not well.

var m = {};
m.n = m;
console.log(`${m}`);

Outputs: "[object Object]"

Looking this over, I think it's safer to go ahead and do a variation on the JSON.stringify performed by the original version:

function stringify(val) {
  try {
    return JSON.stringify(val);
  } catch(_) {
    return '[Circular]';
  }
};
// ...
// ...
`Hostname/IP doesn't match certificate's altnames: ${stringify(reason)}`;

This comment has been minimized.

Copy link
@jasnell

jasnell Oct 20, 2015

Member

Actually, yeah, looking at it, reason is always a string in this case so nevermind 👍

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

I'm a little concerned this may just be code churn, but it does get rid of the need to escape single quotes and stuff.

@Fishrock123 Fishrock123 added the tls label Oct 20, 2015

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

LGTM with a nit. This is the kind of churn I can live with: one less dependency and there are mild performance improvements to be gained.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2015

I'm open to the idea that it might be churn. I would also like to do a perf test to see if switching between util / backticks offers any improvement (could be a perf decrease)

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

It's a wee bit churnish but it's the good kind, I think. LGTM

@jasnell

View changes

lib/tls.js Outdated
cert.subjectaltname);
reason =
`Host: ${host} is not in the cert's altnames:` +
`${cert.subjectaltname}`;

This comment has been minimized.

Copy link
@jasnell

jasnell Oct 20, 2015

Member

You're missing a space between the : and the cert.subjectaltname in the replacement text

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 20, 2015

Author Member

good eye

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

I started a node-test-pull-request CI run but I can't link to it. For some reason there's no build history after September 16...

@evanlucas

This comment has been minimized.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

Looks like they are no longer sorted :[

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2015

Is it just me or is that CI run not doing anything?

edit: it was just me... sigh

facepalm

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

You have to click through to https://ci.nodejs.org/job/node-test-commit/894/ - some windows failures related to buildbots going AWOL, it seems.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2015

@bnoordhuis should we re run the tests?

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 21, 2015

@thealphanerd ... wouldn't hurt to do another run.

@jasnell

This comment has been minimized.

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 21, 2015

Failures appear to be unrelated. Because this touches tls, @indutny ... quick review?

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.
@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2015

just rebased against master

@indutny

This comment has been minimized.

Copy link
Member

commented Oct 27, 2015

LGTM, sorry for delay, the notification was lost in my inbox.

@Trott

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Landed in 6b0c906

@Trott Trott closed this Oct 29, 2015

Trott added a commit that referenced this pull request Oct 29, 2015

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

PR-URL: #3456
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

rvagg added a commit to rvagg/io.js that referenced this pull request Oct 29, 2015

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

PR-URL: nodejs#3456
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

@rvagg rvagg referenced this pull request Oct 29, 2015

Merged

Release proposal: v5.0.0 #3466

@rvagg

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

suggesting this goes into v4.x-staging after having done some time in v5.0.0

@jasnell

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

Let's queue this up for post v4.2.2

MylesBorins added a commit that referenced this pull request Nov 30, 2015

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

PR-URL: #3456
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins added a commit that referenced this pull request Dec 4, 2015

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

PR-URL: #3456
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

@jasnell jasnell referenced this pull request Dec 17, 2015

Closed

Release 4.2.4 Planning #4321

MylesBorins added a commit that referenced this pull request Dec 17, 2015

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

PR-URL: #3456
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins added a commit that referenced this pull request Dec 23, 2015

tls: remove util and calls to util.format
Currently util.format is being used for string templating in tls.
By replacing all of the instances of util.format with backtick
string we can remove the need to require util in tls all together.

PR-URL: #3456
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.