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.

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

util: deprecate most of util.is*() #6235

Closed
wants to merge 1 commit into from
+32 −13
Closed

util: deprecate most of util.is*() #6235

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
39 changes: 26 additions & 13 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,42 +715,51 @@ function reduceToSingleString(output, base, braces) {

// NOTE: These type checking functions intentionally don't use `instanceof`
// because it is fragile and can be easily faked with `Object.create()`.
exports.isArray = Array.isArray;
exports.isArray = internalUtil.deprecate(Array.isArray,
'util.isArray() is deprecated. Use Array.isArray instead.');

function isBoolean(arg) {
return typeof arg === 'boolean';
}
exports.isBoolean = isBoolean;
exports.isBoolean = internalUtil.deprecate(isBoolean,
'util.isBoolean() is deprecated. Use typeof arg === \'boolean\' instead.');

function isNull(arg) {
return arg === null;
}
exports.isNull = isNull;
exports.isNull = internalUtil.deprecate(isNull,
'util.isNull() is deprecated. Use arg === null instead.');

function isNullOrUndefined(arg) {
return arg === null || arg === undefined;
}
exports.isNullOrUndefined = isNullOrUndefined;
exports.isNullOrUndefined = internalUtil.deprecate(isNullOrUndefined,
'util.isNullOrUndefined() is deprecated. Use the following instead:\n' +
'arg === null || arg === undefined');
Copy link
Contributor

Choose a reason for hiding this comment

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

or arg == null

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on == null. Last I looked at the optimizing compiler nullOrUndefined is special cased.


function isNumber(arg) {
return typeof arg === 'number';
}
exports.isNumber = isNumber;
exports.isNumber = internalUtil.deprecate(isNumber,
'util.isNumber() is deprecated. Use typeof arg === \'number\' instead.');

function isString(arg) {
return typeof arg === 'string';
}
exports.isString = isString;
exports.isString = internalUtil.deprecate(isString,
'util.isString() is deprecated. Use typeof arg === \'string\' instead.');

function isSymbol(arg) {
return typeof arg === 'symbol';
}
exports.isSymbol = isSymbol;
exports.isSymbol = internalUtil.deprecate(isSymbol,
'util.isSymbol() is deprecated. Use typeof arg === \'symbol\' instead');

function isUndefined(arg) {
return arg === undefined;
}
exports.isUndefined = isUndefined;
exports.isUndefined = internalUtil.deprecate(isUndefined,
'util.isUndefined() is deprecated. Use arg === undefined instead.');

function isRegExp(re) {
return binding.isRegExp(re);
Expand All @@ -760,7 +769,9 @@ exports.isRegExp = isRegExp;
function isObject(arg) {
return arg !== null && typeof arg === 'object';
}
exports.isObject = isObject;
exports.isObject = internalUtil.deprecate(isObject,
'util.isObject() is deprecated. Use the following instead:\n' +
'arg !== null && typeof arg === \'object\'');

function isDate(d) {
return binding.isDate(d);
Expand All @@ -772,15 +783,17 @@ exports.isError = isError;
function isFunction(arg) {
return typeof arg === 'function';
}
exports.isFunction = isFunction;
exports.isFunction = internalUtil.deprecate(isFunction,
'util.isFunction() is deprecated. ' +
'Use typeof arg === \'function\' instead.');

function isPrimitive(arg) {
Copy link
Member

Choose a reason for hiding this comment

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

I would separate out the change to isPrimitive to a separate commit or PR.

return arg === null ||
typeof arg !== 'object' && typeof arg !== 'function';
return binding.isPrimitive(arg);
}
exports.isPrimitive = isPrimitive;

exports.isBuffer = Buffer.isBuffer;
exports.isBuffer = internalUtil.deprecate(Buffer.isBuffer,
'util.isBuffer() is deprecated. Use Buffer.isBuffer() instead.');


function pad(n) {
Expand Down
6 changes: 6 additions & 0 deletions src/node_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ using v8::Value;
#undef V


static void IsPrimitive(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(!args[0]->IsObject() && !args[0]->IsExternal());
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you come up with this check? Does it match what the v8 native %IsPrimitive() checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Credit due to @trevnorris

%IsPrimitive() isn't exposed anymore so as far as I understand this is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, looks like v8 does something different now?

node/deps/v8/src/objects-inl.h

Lines 898 to 900 in 8c48a26

bool Object::IsPrimitive() const {
return IsSmi() || HeapObject::cast(this)->map()->IsPrimitiveMap();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I saw that too and it is mostly what prompted my question.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible to do this in javascript, is this significantly faster?

Copy link
Contributor

Choose a reason for hiding this comment

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

@benjamingr The whole point of this is to do it at the C++ layer instead of js because there is no reliable way to determine it in js (especially in a way that agrees with how v8 internally determines whether a value is a primitive or not).

Copy link
Member

Choose a reason for hiding this comment

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

There are lots of cases to test it in JS:

const tester = Symbol();
const tested = maybePrimitiveMaybeObject();
tested[tester] = 'Foo';
if(tested[tester]) {
    delete tested[tester]; 
    console.log("Object");
} else {
    console.log("Primitive");
}

Or:

var isPrimitive = (function() { return this !== maybePrimitive; }).call(maybePrimitive); // except in strict mode

Or without allocating a new object

    var isObject = (typeof maybeObject === "object" && maybeObject !== null) || (typeof maybeObject === "function");

Or:

    var isPrimitive = typeof o === "number" || typeof o === "string" || typeof o === "boolean" || typeof o === "undefined" || typeof o === "null" || typeof o === "symbol";

Which is why I asked if this is about performance. Is it?

}


static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -84,6 +89,7 @@ void Initialize(Local<Object> target,

env->SetMethod(target, "getHiddenValue", GetHiddenValue);
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
env->SetMethod(target, "isPrimitive", IsPrimitive);
}

} // namespace util
Expand Down