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

Confusing documentation or bad implementation for util.isObject #743

Closed
seishun opened this issue Feb 6, 2015 · 12 comments
Closed

Confusing documentation or bad implementation for util.isObject #743

seishun opened this issue Feb 6, 2015 · 12 comments
Labels
doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.

Comments

@seishun
Copy link
Contributor

seishun commented Feb 6, 2015

The documentation for util.isObject states:

Returns true if the given "object" is strictly an Object. false otherwise.

What exactly does "strictly an Object" mean? That the type of the argument is Object? Nope:

> var f = function(){}
undefined
> util.isObject(f)
false

Or maybe it does an instanceof check? That also isn't the case:

> var o = Object.create(null)
undefined
> o instanceof Object
false
> util.isObject(o)
true

Does it check that the [[Class]] internal property is "Object"? Also no:

> Object.prototype.toString.call(JSON)
'[object JSON]'
> util.isObject(JSON)
true

Either the docs should be updated to clarify the behavior, or the implementation should be fixed. I would vote for the latter, otherwise the function name is confusing.

@seishun seishun added util Issues and PRs related to the built-in util module. doc Issues and PRs related to the documentations. labels Feb 6, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

It checks arg !== null && typeof arg === 'object' to be exact.

EDIT: I think "strictly" is thrown in there because typeof returns 'object' for null.

@vkurchatkin
Copy link
Contributor

yeah, the wording is really confusing

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

Would this work for you:

Returns true if object is not null and typeof object evaluates to 'object'. false otherwise.

@seishun
Copy link
Contributor Author

seishun commented Feb 6, 2015

Kinda, but it's generally a bad sign when the documentation basically rephrases the source code. It would be better to change the implementation to check whether Type(arg) is Object, to be consistent with the other util.is* functions.

@ceejbot
Copy link

ceejbot commented Feb 12, 2015

The implementation does check whether typeof arg is object, and in the case of a function, it isn't. Both the underscore and lodash implementations of isObject() have additional checks to return true when the argument is a function, since that's the expected behavior. So I also think changing the implementation is more useful than changing the docs here.

@ceejbot
Copy link

ceejbot commented Feb 12, 2015

(Util is frozen, I note, so doc change it'll have to be.)

@vkurchatkin
Copy link
Contributor

@ceejbot it's not about util being frozen, but about semver. Bumping major version because of isObject one-liner is kind of funny

@seishun
Copy link
Contributor Author

seishun commented Feb 12, 2015

Util is frozen

The documentation for util.isObject doesn't state that it returns false for functions, so I would consider it a bug fix. Besides, util.isObject didn't even officially exist before io.js v1.0.0, so it's unlikely a lot of people are relying on this broken behavior.

@targos
Copy link
Member

targos commented Feb 12, 2015

I do not think that the expected behavior for everyone is to return true for functions. There is util.isFunction for that. util.isObject can be used to check function arguments and the common case is to differentiate functions from plain objects.
IMO if there is a "bug" to fix, it is in the doc.

@seishun
Copy link
Contributor Author

seishun commented Feb 12, 2015

@targos

I do not think that the expected behavior for everyone is to return true for functions.

It's the expected behavior for anyone who knows what an object is.

There is util.isFunction for that.

util.isPrimitive returns true for all primitives, util.isString returns true just for strings. It's the same thing with objects and functions.

util.isObject can be used to check function arguments and the common case is to differentiate functions from plain objects.

That's what util.isFunction is for.

ceejbot pushed a commit to ceejbot/io.js that referenced this issue Feb 12, 2015
This fixes issue nodejs#743.

Brought the behavior of util.isObject() in line with expected
behavior, inspired by utility libraries like underscore & lodash.
In particular, it now returns true for functions, as expected.

Added eight tests for isObject() to ensure it behaves as
expected for a wide variety of input, including cases where it
is expected to return false.
@trevnorris
Copy link
Contributor

IMO the docs should change to explain that an Object is anything that isn't a primitive. Though that probably doesn't clear up a lot.

Fishrock123 added a commit to Fishrock123/node that referenced this issue Mar 31, 2015
Proposed functionality fix containing prior discussion:
nodejs#822

Fixes: nodejs#743
PR-URL: nodejs#1295
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brendan Ashworth <[email protected]>
@Fishrock123
Copy link
Contributor

Fixed by a doc fix in 245ba1d

See later comments in #822 (comment) for doc vs functionality part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

7 participants