Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Made isPlainObject() supporting null, undefined, and window values on…

… IE too. Also added some related tests. Fixes #5669.
  • Loading branch information...
commit 148fb7ba8e992dd70c64cdc6a1c6f643fd1ba160 1 parent 27d65b5
@rkatic rkatic authored jeresig committed
Showing with 20 additions and 5 deletions.
  1. +6 −4 src/core.js
  2. +14 −1 test/unit/core.js
View
10 src/core.js
@@ -425,19 +425,21 @@ jQuery.extend({
},
isPlainObject: function( obj ) {
- if ( toString.call(obj) !== "[object Object]" || typeof obj.nodeType === "number" ) {
+ // Must be an Object.
+ // Because of IE, we also have to check the presence of the constructor property.
+ if ( !obj || toString.call(obj) !== "[object Object]" || !("constructor" in obj) ) {
@kangax
kangax added a note

I'm a bit confused about what this method really checks for. Is it instances of Object? Host or native? Or both? Should an object in question have Object as direct parent in its prototype chain or at any level? What is an actual intent here and why are there so many jumps and hoops to go through?

Right now I see a collection of somewhat unrelated checks 1) if object type converts to false, it's not a plain object (fwiw, all native Object objects should type convert to true, but host objects can easily type convert to false); 2) if object's [[Class]] is not "Object", it's not a plain object; 3) if there's no constructor property anywhere in object or its prototype chain, it's not a plain object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return false;
}
- // not own constructor property must be Object
+ // Not own constructor property must be Object
if ( obj.constructor
&& !hasOwnProperty.call(obj, "constructor")
&& !hasOwnProperty.call(obj.constructor.prototype, "isPrototypeOf") ) {
return false;
}
- //own properties are iterated firstly,
- //so to speed up, we can test last one if it is own or not
+ // Own properties are enumerated firstly, so to speed up,
+ // if last one is own, then all properties are own.
@gnarf Owner
gnarf added a note

This comment inspired a Comment Haiku

@rkatic
rkatic added a note

I am flattered, I guess...

@gnarf Owner
gnarf added a note

:) It seemed kinda like a poem, so I made it a haiku!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
var key;
for ( key in obj ) {}
View
15 test/unit/core.js
@@ -204,12 +204,22 @@ test("trim", function() {
});
test("isPlainObject", function() {
- expect(7);
+ expect(14);
stop();
// The use case that we want to match
ok(jQuery.isPlainObject({}), "{}");
+
+ // Not objects shouldn't be matched
+ ok(!jQuery.isPlainObject(""), "string");
+ ok(!jQuery.isPlainObject(0) && !jQuery.isPlainObject(1), "number");
+ ok(!jQuery.isPlainObject(true) && !jQuery.isPlainObject(false), "boolean");
+ ok(!jQuery.isPlainObject(null), "null");
+ ok(!jQuery.isPlainObject(undefined), "undefined");
+
+ // Arrays shouldn't be matched
+ ok(!jQuery.isPlainObject([]), "array");
// Instantiated objects shouldn't be matched
ok(!jQuery.isPlainObject(new Date), "new Date");
@@ -231,6 +241,9 @@ test("isPlainObject", function() {
// DOM Element
ok(!jQuery.isPlainObject(document.createElement("div")), "DOM Element");
+
+ // Window
+ ok(!jQuery.isPlainObject(window), "window");
var iframe = document.createElement("iframe");
document.body.appendChild(iframe);

10 comments on commit 148fb7b

@rkatic

In theory, plain objects are all "host" objects obj that satisfies Object.getPrototypeOf(obj) === Object.prototype where Object is from the same window of obj.

1) if object type converts to false, it's not a plain object (fwiw, all native Object objects should type convert to true, but host objects can easily type convert to false);

Can you give an example?

2) if object's [[Class]] is not "Object", it's not a plain object;

Of course. Instances of other "classes" will not be considered plain objects.

3) if there's no constructor property anywhere in object or its prototype chain, it's not a plain object.

Of course. Also in IE "native" objects have not constructor property which was helpful to detect strange behavior in IE where toString.call(aNative) === [object Object]. Unfortunately it seams it was not working in the test suite, so !("constructor" in obj) was replaced with obj.nodeType || obj.setInterval.

@jdalton

@rkatic

1) ECMA section 4.3.8: host object - object supplied by the host environment to complete the execution environment of ECMAScript. NOTE Any object that is not native is a host object.

2) I believe kangax is saying because host objects are implementation dependent, they may/may not internally provide the same GetValue(expr) results, when evaluating the not (!) operator, as that of a native object.

3) Simple truthy checks are pretty weak, you might additionally check for the expected types of each.

@rkatic

1) ECMA section 4.3.8: host object - object supplied by the host environment to complete the execution environment of ECMAScript. NOTE Any object that is not native is a host object.

Seems that I switched the two definitions. Sorry guys.
However, I am not absolutely sure that native/host distinction is exact the same for all browsers. For that reason I used quotes.

So the corrected definition of "plain object" would be: Any NATIVE object obj that satisfies Object.getPrototypeOf(obj) === Object.prototype where Object is from the same window of obj.

2) I believe kangax is saying because host objects are implementation dependent, they may/may not internally provide the same GetValue(expr) results, when evaluating the not (!) operator, as that of a native object.

Since we need NATIVE objects only, I think that the not-operator is quite safe here.

3) Simple truthy checks are pretty weak, you might additionally check for the expected types of each.

IF truthy checks are still weak to you, than we can simply replace !obj with obj == null.

@rkatic

Aaaaaaaah, you and the specification have confused me. I haven't switched native/host definitions. Am I?
My English is not too good, and the ECMA specification is not always clear to me. Maybe I am not the only one...

If the whole point is that negated plain-objects are not always negative, then a simple replacement of !obj with obj == null would be enough...

@jdalton

@rkatic on truthy checks I was talking about obj.nodeType || obj.setInterval

@rkatic

Well, we can't easily (with no surplus costs) make additionally check on obj.setInterval. Neither typeof nor isFunction() would be enough.
Is there a more robus,t but still light, way to check if an object is a Window?

Eventually we can do typeof obj.nodeType === "number", but according to the jQuery core conventions, nodes are tested with a simple obj.nodeType.

@jdalton

@rkatic Host objects can have characteristics of native objects such as Object.getPrototypeOf(obj) === Object.prototype but that is not consistent or guaranteed across browsers/environments.

@rkatic

Host objects can have characteristics of native objects such as Object.getPrototypeOf(obj) === Object.prototype but that is not consistent or guaranteed across browsers/environments.

Of course. It is why I used host/native as an condition of the definition.
To be more clearer, we can say that "plain objects" are all "user" objects definable with the literal {...} notation. In fact, first version of the function was named isLiteralObject. Later it was renamed to isPlainObject.

@kangax

Sorry for getting back so late. I somehow forgot about this "thread". On to the points.

There are native and host objects in ECMAScript. Native objects are supplied independent of host environment. {}, [1,2,3], function foo(){}, Object and RegExp.prototype — are all examples of native objects. Some of native objects are also called built-ins — these are the ones specified in ECMA — Object, Function, Number, parseInt, Math.max, etc. And then there are host objects — those provided by host enviornment (e.g. exposed DOM bindings in browsers are obviously host objects — document, document.getElementById, etc.)

When you say "plain object" is that which is defined with the literal {}, it sounds akward and still unclear to me. {} only denotes how something is created, not what it really is :) new Object(), for example, creates identical type of object, given that Object constructor is not overridden or shadowed. new Object has nothing to do with literal notation, yet it creates an object similar to the one created via {} (i.e. with same internal [[Prototype]] and [[Class]]). If I create my object via new Object() is it not a plain object anymore? It wasn't created with {} after all ;) And what if I created an object with {} and then changed its [[Prototype]] to reference Array.prototype? {}-based definition doesn't seem like a good idea.

We can't detect object's [[Prototype]] in standard ES3 (without __proto__ extension or ES5 Object.getPrototypeOf), so I don't see how it's possible to satisfy o is <plain object> if Object.getPrototypeOf(o) == Object.prototype condition. And if that's the only goal, why is there a [[Class]] check in current implementation? Does it mean that besides [[Prototype]] == Object.prototype, "plain object" should also have [[Class]] == "Object"?

It's still hard to explain what "plain object" really is. And the reason I'm bringing this up is that when it's hard to explain what method really does, it usually means that something went wrong and probably needs to be rethought.

@rkatic

kangax, thanks for clarifying which objects are "native" and which are "host". Finally it is obvious that "plain objects" are native, not built-in, objects.

When I said that "plain objects" are DEFINABLE with the literal {...} notation, then I intend that all "plain objects" can be described with a {...} notation.
Created objects with the new Object() notation can be entirely described with the {} notation.

I am conscious that an ideal implementation of isPlainObject is impossible in ES3 (probably even in ES5...), but the current one is a good compromise between accuracy and complexity. I can write several test cases that would make isPlainObject to fail, but that cases would be really unusual in real world. Currently the most slack part of the implementation is probably obj.nodeType || obj.setInterval and maybe I have a more robust (still not ideal) solution for it, but have to make some additional investigations...

And if that's the only goal, why is there a [[Class]] check in current implementation? Does it mean that besides [[Prototype]] == Object.prototype, "plain object" should also have [[Class]] == "Object"?

Well, yes.

It's still hard to explain what "plain object" really is. And the reason I'm bringing this up is that when it's hard to explain what method really does, it usually means that something went wrong and probably needs to be rethought.

Well, maybe someone can explain it better than me. John?
Even if it is not simple to explain, it is still needed to distinguish "option hashes" from other objects.
I AM of idea that it was better to avoid any need of such hard-to-define functionalities, but John probably was of idea that it is worthwhile.

Please sign in to comment.
Something went wrong with that request. Please try again.