util: fixed behavior of isObject() #822
Conversation
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.
vkurchatkin
added
semver-major
|
LGTM |
| @@ -543,7 +543,8 @@ function isRegExp(re) { | |||
| exports.isRegExp = isRegExp; | |||
|
|
|||
| function isObject(arg) { | |||
| return arg !== null && typeof arg === 'object'; | |||
| var type = typeof arg; | |||
| return type === 'function' || type === 'object' && !!arg; | |||
There was a problem hiding this comment.
I think it might still be faster to do arg !== null.
There was a problem hiding this comment.
That doesn't change the behavior, so if it's faster, happy to make the change.
Double negation considered slower than a straight null check.
|
Thanks for the contribution! As @vkurchatkin noted, this change is potentially breaking, and will likely have to go out in a major version release. I will chat with the TC about doing a major-version release, so I'll leave this PR open until there's a decision made. I'll comment on this PR with updates. |
|
resolution from last TC meeting on this was:
|
|
Actually, I'm -1 on changing |
|
To be entirely honest, I have no idea why these utility functions are in core at all if they're going to be wrong-- I wasn't aware this existed until I saw the bug, and now that I know it exists I will never, ever be using it. The feature shouldn't have been added when it was so poorly specced. That, however, is ancient history and you're stuck with it. Doc & deprecate if fixing is impossible. |
And neither do I... |
|
What's the verdict? Semver-major or reject and update the documentation? |
|
Since we already have +1 docs |
|
Reject & update the docs. Too much breakage from io.js right now to risk it. I would also deprecate things like this that (arguably) do not belong in node core but are instead much better provided in userland. |
|
Closing. Doc PR open at #1295. |
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]>
This fixes issue #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.
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.