_.isPlainObject seems totally broken

[tjwebb]

assert(_.isPlainObject({ a: _.isZero }));
assert(_.isPlainObject({ }));

These fail. I can't find any sane value that will pass. Can someone give me a quick sanity check?

Node 0.10, underscore 1.6

[tjwebb]

It seems to fail at module scope in Node:

> { }.constructor === module.Object
false

re: https://github.com/documentcloud/underscore-contrib/blob/master/underscore.function.predicates.js#L39

[tjwebb]

So, my goal was to convert all my custom-crazy predicates to use underscore-contrib versions in my congruence module (https://github.com/tjwebb/congruence), which is going pretty well. But I think the isPlainObject function is not quite right. My implementation looks like this, it seems more reliable, and I think it covers all the same cases:

https://github.com/tjwebb/congruence/blob/master/index.js#L15

Let me know what you think.

[joshuacc]

@tjwebb I will take a look in Node as soon as I'm able.

Unfortunately, your implementation doesn't quite cover all the cases that isPlainObject should. Specifically, it doesn't check whether the object is a custom type. _.isPlainObject(new Foo()) should return false.

[tjwebb]

Perhaps our intended use cases are different. I'll argue that new Foo() actually is a plain object, just with a different value set for constructor. Because in your implementation, this would also fail:

_.isPlainObject({ constructor: 'happy' });

Even though that's a perfectly fine "plain" object. Checking constructor as a means to determine purity is not a reliable test.

Explicitly checking for _.not(_.isArray(val)) and others is legit because I filter out all other built-in types that are objects but are not equivalent to the typical new Object-like construction.

[joshuacc]

Totally agree that this is a problem with the implementation. The intention of _.isPlainObject however, is to test that an object is a "plain" object as opposed to a "subtype" object. I want to move toward a more robust solution, possibly similar to the implementation in LoDash.

[tjwebb]

Ok. _.isPlainObject could certainly resemble the lodash implementation; but yea, as it stands, I'd call the current one broken. So I guess what I'm actually proposing is a new feature, similar to my _.isObjectStrict() function I link to above. Let me know if you'd accept a PR with that feature.

[joshuacc]

@tjwebb I'd certainly consider a pull request with a clear rationale. But as of right now, I'm not sure what isObjectStrict gives you that (a more robust) isPlainObject wouldn't. If a better implementation of isPlainObject could meet your needs, I'd suggest looking into improving that method instead.

[joshuacc]

Notes for specific improvements to isPlainObject:

• Don't overwrite an existing isPlainObject (Lo-Dash's version is more robust right now.)
• Use Object.getPrototypeOf as the preferred approach for comparison
• Take into account Object.create(null) objects

[jgonggrijp]

I have a good implementation ready in the Fusion project. I'll transfer it to Contrib after modularization.