Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13715 closed bug (duplicate)

isPlainObject won't work correctly when constructor property is changed

Reported by: fhcj2 Owned by:
Priority: undecided Milestone: None
Component: unfiled Version: 1.9.1
Keywords: Cc:
Blocked by: Blocking:

Description

When you instantiate a non Object class (not a plain object), and add it its own constructor property, the function will take it for a Plain Object.

See it here: http://jsfiddle.net/fhcj2/J8Yrx/

Change History (11)

comment:1 Changed 7 years ago by fhcj2

I think it's cause of this block of code:

if ( obj.constructor &&
		!core_hasOwn.call( obj.constructor.prototype, "isPrototypeOf" ) ) {
	return false;
}

Since every object has its own prototype, which leads to the constructor who created it, I think it could be changed for:

if (!core_hasOwn.call(Object.getPrototypeOf(obj).constructor.prototype, "isPrototypeOf") ) {
	return false;
}

I tested it and it fixed the problem. What do you guys think? Is it ready for a pull request?

Last edited 7 years ago by fhcj2 (previous) (diff)

comment:2 Changed 7 years ago by Rick Waldron

Resolution: duplicate
Status: newclosed

Duplicate of #13571.

comment:3 Changed 7 years ago by fhcj2

Actually this issue is about jQuery 2.0 (had just cloned it from the repository, but forgot to change it here, sry..)

But anyway this is an issue that happens both in 2.0 and 1.9.1 versions of jQuery, and I think it's not related to "Ticket #13571", since it covers a specific situation when the object is added a "constructor" property. Would you explain why you considered it as a duplicate? =\

comment:4 Changed 7 years ago by m_gol

Your jsFiddle specificely uses jQuery 1.9.1, not 2.0.x. Also, if you change the version, your test no longer returns true in the last case.

Are you sure you have a problem with specifically jQuery 2.0? Because it doesn't seem so.

comment:5 Changed 7 years ago by fhcj2

Just noticed that now that you said... But I even recloned it right now and the error still occurs on the compiled (by grunt) version from the repository. It says in the code:

/*!
 * jQuery JavaScript Library v2.0.0-pre
 * http://jquery.com/
 *
 * Includes Sizzle.js
 * http://sizzlejs.com/
 *
 * Copyright 2005, 2013 jQuery Foundation, Inc. and other contributors
 * Released under the MIT license
 * http://jquery.org/license
 *
 * Date: 2013-04-03
 */

Would that "2.0.0b1" or "2.x" from jsfiddle be any different from this one? Ty for the reply btw =]

comment:6 Changed 7 years ago by m_gol

jQuery edge 2.x is code from code.jquery.com/jquery-git2.js which is always the latest jQuery compilation from master. If you think your compiled code does sth different, try comparing those files (using, for example, some command line tool like diff).

Isn't it possible that your browser cached jQuery 1.9? I'd suppose this could be the reason as the jsFiddle clearly shows the problem doesn't exist in jQuery 2.0

Last edited 7 years ago by m_gol (previous) (diff)

comment:7 Changed 7 years ago by Rick Waldron

I'm not sure what part is unclear, both are directly related to detecting "plain object"-ness when an object's constructor has been changed—either by defining the prototype with an object (and thereby changing the prototype.constructor property) or by adding an own property that happens to be called "constructor". Once you modify a special property, such as "constructor", all bets are off.

comment:8 Changed 7 years ago by fhcj2

Sry, guys.. My mistake. That example would only cover the error in 1.x really. Made another jsfiddle script using the 2.x jQuery version. http://jsfiddle.net/fhcj2/cNNA4/ Wouldn't it be an error? The change I proposed would fix it too...

comment:9 Changed 7 years ago by m_gol

Your change moves the check one level below. It can fix your specific use case but it's not a golden solution. For one, your code will completely break when invoked on object created via Object.create(null) - null prototype doesn't have a constructor field, obviously.

comment:10 Changed 7 years ago by fhcj2

Didn't remember that function.. But I think perhaps a simple "!= null" would solve that problem, right? since it takes an object to reach this code.

var objProto = Object.getPrototypeOf(obj);
if (objProto != null && !core_hasOwn.call(objProto.constructor.prototype, "isPrototypeOf") ) {
	return false;
}

Although it wouldn't cover another (possible)problem which also happens in jQuery 2.x: http://jsfiddle.net/fhcj2/jwpqK/

Since an object created this way seems to behave exactly as a "plain object", I'm not sure if it's not right for it to be identified as such. In the case it is not considered a bug, maybe this code would suffice?

comment:11 Changed 7 years ago by m_gol

Well, this additional check doesn't help in all cases since you just moved it one level lower. If instead of assigning MyClass.prototype to null you assign it to Object.create(null), the code will throw. Sure, it's catched later but using exceptions for flow control is not a good practice. Not to mention your code no longer recognizes {} as a plain object which kind of misses the point...

Like rwaldron said, when you change the prototype field of the constructor function or the constructor field of an object, all weird kinds of things could happen.

P.S. There is no need to type $(document).ready by hand in jsFiddle, since you chose onLoad in the left jsFiddle pane all the code is invoked after window load event occured (and, thus, jQuery document ready, too).

Note: See TracTickets for help on using tickets.