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

Jump to bottom

util: fixed behavior of isObject() #822

Closed
wants to merge 2 commits into from
Closed

util: fixed behavior of isObject() #822

wants to merge 2 commits into from

Conversation

ceejbot
Copy link

@ceejbot ceejbot commented Feb 12, 2015

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.

util: fixed behavior of isObject()
b0ed45b 
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 vkurchatkin added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 12, 2015
@tellnes
Copy link
Contributor

tellnes commented Feb 12, 2015

LGTM

cjihrig
cjihrig reviewed Feb 12, 2015
View reviewed changes
lib/util.js
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might still be faster to do arg !== null.

Copy link
Author

Choose a reason for hiding this comment

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

That doesn't change the behavior, so if it's faster, happy to make the change.

Perf tweak as suggested by @cjihrig
aac9a5e 
Double negation considered slower than a straight null check.
@chrisdickinson
Copy link
Contributor

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.

@rvagg rvagg mentioned this pull request Feb 18, 2015
Closed
@chrisdickinson chrisdickinson added this to the 2.0.0 milestone Feb 18, 2015
@rvagg
Copy link
Member

rvagg commented Mar 4, 2015

resolution from last TC meeting on this was:

tag this issue as a milestone for now and punt until we have more substantive changes

#1051

@rvagg rvagg removed the tc-agenda label Mar 4, 2015
@chrisdickinson chrisdickinson mentioned this pull request Mar 4, 2015
Closed
@piscisaureus
Copy link
Contributor

Actually, I'm -1 on changing util.isObject behavior. It's a bikeshed that makes it necessary for an enormous number of people to review their code. It'd be better to clarify the docs and call it a day.

@ceejbot
Copy link
Author

ceejbot commented Mar 6, 2015

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.

@piscisaureus
Copy link
Contributor

To be entirely honest, I have no idea why these utility functions are in core at all

And neither do I...

@bnoordhuis
Copy link
Member

What's the verdict? Semver-major or reject and update the documentation?

@Fishrock123
Copy link
Contributor

Since we already have isFunction() i'd still say it makes more sense to keep the original behaviour. Probably far less painful.

+1 docs

@ceejbot
Copy link
Author

ceejbot commented Mar 21, 2015

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.

@Fishrock123 Fishrock123 removed this from the 2.0.0 milestone Mar 22, 2015
@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Mar 22, 2015
@Fishrock123 Fishrock123 mentioned this pull request Mar 30, 2015
Merged
@Fishrock123
Copy link
Contributor

Closing. Doc PR open at #1295.

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 31, 2015
@Fishrock123
doc: fix util.isObject documentation
245ba1d 
Proposed functionality fix containing prior discussion:
nodejs#822

Fixes: nodejs#743
PR-URL: nodejs#1295
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
This was referenced Mar 31, 2015
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.