lib: reduce util.is*() usage #647

cjihrig · 2015-01-29T01:44:09Z

Many of the util.is*() methods used to check data types simply compare against a single value or the result of typeof. This commit replaces calls to these methods with equivalent checks. This commit does not touch calls to the more complex methods (isRegExp(), isDate(), etc.), which are infrequently used anyway.

Closes #607

brendanashworth · 2015-01-29T01:56:16Z

This will conflict with the changes in #601 and #605, by the way.

Fishrock123 · 2015-01-29T02:35:44Z

I think we should document which conventions we will be using first.

cjihrig · 2015-01-31T00:51:26Z

Are docs the only thing holding this up? I think @chrisdickinson was going to write something up.

chrisdickinson · 2015-01-31T01:38:57Z

I'll have a PR this coming week. I would not consider that a blocker for landing this.

bnoordhuis · 2015-01-31T07:48:08Z

lib/_http_client.js

@@ -27,7 +27,8 @@ function ClientRequest(options, cb) {
  var defaultAgent = options._defaultAgent || Agent.globalAgent;
  if (agent === false) {
    agent = new defaultAgent.constructor();
-  } else if (util.isNullOrUndefined(agent) && !options.createConnection) {
+  } else if ((agent === null || agent === undefined) &&


Can be shortened to agent == null.

OK. I'll update these. I was trying to avoid using ==.

On second thought, I'll leave it to your good judgment. Doing the explicit checks is a little more efficient than == because they don't generate an IC.

bnoordhuis · 2015-01-31T08:32:20Z

Do people have opinions on what is better, Array.isArray(x) or x instanceof Array? I ask because of the following:

> var x = []; x.__proto__ = {};
{}
> Array.isArray(x)
true
> x instanceof Array
false

With Array.isArray(), you don't know if Array prototype methods are available on the instance.

Second question: should String objects be considered strings? Right now, only primitive strings are accepted. new String('foo') is either rejected or (in the binding layer) asserts.

rvagg · 2015-01-31T09:26:03Z

@chrisdickinson will your style-guide cover the specifics of when to use what type of type checking? There was a suggestion in the TC meeting to bring @micnic in to help with that effort since he raised the concern originally in #607. So, if it's not straight-forward and you'd like input, then you could try and rope him in and use this PR as a basis for coming up with that specific list of rules for a style-guide.

vkurchatkin · 2015-01-31T10:05:50Z

@bnoordhuis there is an opposite example: vm.runInNewContext('[]') instanceof Array. Returns false, though it actually IS an array and has all prototype methods available.

cjihrig · 2015-01-31T15:08:39Z

FWIW, the current util.isArray() is just an alias to Array.isArray().

Many of the util.is*() methods used to check data types simply compare against a single value or the result of typeof. This commit replaces calls to these methods with equivalent checks. This commit does not touch calls to the more complex methods (isRegExp(), isDate(), etc.).

cjihrig · 2015-01-31T16:38:53Z

@bnoordhuis I've updated the isObject() checks.

bnoordhuis · 2015-01-31T17:20:59Z

lib/child_process.js

@@ -893,7 +893,7 @@ function _validateStdio(stdio, sync) {
        wrapType: getHandleWrapType(handle),
        handle: handle
      });
-    } else if (util.isBuffer(stdio) || util.isString(stdio)) {
+    } else if ((stdio instanceof Buffer) || typeof stdio === 'string') {


The parens are superfluous here.

bnoordhuis · 2015-01-31T17:22:50Z

LGTM sans a tiny style nit. Land at will if https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/142/ is happy.

Many of the util.is*() methods used to check data types simply compare against a single value or the result of typeof. This commit replaces calls to these methods with equivalent checks. This commit does not touch calls to the more complex methods (isRegExp(), isDate(), etc.). Fixes: #607 PR-URL: #647 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

cjihrig · 2015-02-01T04:54:36Z

Landed in 6ac8bdc

This commit does various changes to cleanup and prep the 8ff6b81 for merging, which include updating the commit to follow new codebase guidelines. The following are the changes: doc/api/http.markdown: - document options lib/http.js: - no changes lib/_http_server.js - changes regarding isObject (nodejs#647) and hasOwnProperty (nodejs#635) - take tls option rather than guessing based on options test/simple/test-https-from-http.js: - moved to parallel directory, crypto test, removed copyright banner test/parallel/test-http-server.js: - adjust for tls option

bnoordhuis reviewed Jan 31, 2015
View reviewed changes

cjihrig closed this Feb 1, 2015

cjihrig deleted the util-is branch February 1, 2015 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: reduce util.is*() usage #647

lib: reduce util.is*() usage #647

cjihrig commented Jan 29, 2015

brendanashworth commented Jan 29, 2015

Fishrock123 commented Jan 29, 2015

cjihrig commented Jan 31, 2015

chrisdickinson commented Jan 31, 2015

bnoordhuis Jan 31, 2015

cjihrig Jan 31, 2015

bnoordhuis Jan 31, 2015

bnoordhuis commented Jan 31, 2015

rvagg commented Jan 31, 2015

vkurchatkin commented Jan 31, 2015

cjihrig commented Jan 31, 2015

cjihrig commented Jan 31, 2015

bnoordhuis Jan 31, 2015

bnoordhuis commented Jan 31, 2015

cjihrig commented Feb 1, 2015

lib: reduce util.is*() usage #647

lib: reduce util.is*() usage #647

Conversation

cjihrig commented Jan 29, 2015

brendanashworth commented Jan 29, 2015

Fishrock123 commented Jan 29, 2015

cjihrig commented Jan 31, 2015

chrisdickinson commented Jan 31, 2015

bnoordhuis Jan 31, 2015

Choose a reason for hiding this comment

cjihrig Jan 31, 2015

Choose a reason for hiding this comment

bnoordhuis Jan 31, 2015

Choose a reason for hiding this comment

bnoordhuis commented Jan 31, 2015

rvagg commented Jan 31, 2015

vkurchatkin commented Jan 31, 2015

cjihrig commented Jan 31, 2015

cjihrig commented Jan 31, 2015

bnoordhuis Jan 31, 2015

Choose a reason for hiding this comment

bnoordhuis commented Jan 31, 2015

cjihrig commented Feb 1, 2015