Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#13571 closed bug (wontfix)

"isPlainObject" losts compatibility against 1.9.1

Reported by: ystk_skm (brilliantpenguin@… Owned by: Rick Waldron
Priority: undecided Milestone: None
Component: unfiled Version: 2.0b2
Keywords: Cc:
Blocked by: Blocking:

Description

You are modifying "isPlainObject" method and it is now losts compatibility against 1.9.1.
When some prototype is set via Object, the prototype have "isPrototypeOf" method. Please check an example for the matter:

http://localhost/cloudplus.me/EasyCoding/

# my checking conditon:

Google Chrome 25.0.1364.152 (Official Build 185281)
OS Mac OS X
WebKit 537.22 (@144108)
JavaScript V8 3.15.11.16

Change History (17)

comment:1 Changed 7 years ago by ystk_skm (brilliantpenguin@…

I'm sorry for my mistake. The correct url is: http://cloudplus.me/EasyCoding/

comment:2 Changed 7 years ago by ajpiano

#13572 is a duplicate of this ticket.

comment:3 Changed 7 years ago by Rick Waldron

Owner: set to Rick Waldron
Status: newassigned

comment:4 Changed 7 years ago by Rick Waldron

Confirm that this is a "regression", but jQuery 1.9.x was wrong.

The constructed object _IS_ a plain object, because the code is paving over the constructor's prototype:

function C() {}
C.prototype = {};
c = new C();
console.log(c.constructor);
// > function Object() { [native code] }

If the program resets the constructor, the expected results:

function C() {}
C.prototype = {
  constructor: C
};
c = new C();
console.log(c.constructor);
// > function C() {}

In jQuery.isPlainObject's case, the OPs example code only works in jQuery 1.9.x because it has properties on the object that was used as the prototype, which is what jQuery 1.9.x used to determine property ownership and therefore detect possible inheritance chains.

Everything works the way you want it to until you try to test this:

var c, extended;
function C() {}
C.prototype = {};
c = new C();

extended = jQuery.extend( true, {}, {
  target: c
});

strictEqual( extended.target, c, "Deep extended objects retain identity" );

... Without any properties on that prototype, the jQuery 1.9 way will fail as well: http://jsfiddle.net/rwaldron/7Jg8Q/

Unfortunately, the regression stands and I'll have to restore the behaviour, but it will still be broken (ie. it's correct, but the expectations of the calling code is wrong).

comment:5 Changed 7 years ago by Rick Waldron

Resolution: fixed
Status: assignedclosed

Fixes #13571. jQuery.isPlainObject 1.9.x compatibility

Signed-off-by: Rick Waldron <waldron.rick@…>

Changeset: 8d1c42296fefc4e79189b6c2d78a8a78fdd9576d

comment:6 Changed 7 years ago by Rick Waldron

Resolution: fixed
Status: closedreopened

comment:7 Changed 7 years ago by Rick Waldron

We've decided that this breaking change will remain in 2.x, but will be accompanied by documentation explaining the issue encountered here.

Reverted: https://github.com/jquery/jquery/commit/5c82d36f194a854a19f81bebd3ed1a6e7559c811

comment:8 Changed 7 years ago by ystk_skm <brilliantpenguin@…>

rwaldron

I can accept your decision easily.

comment:9 Changed 7 years ago by Rick Waldron

wow, thanks for being so reasonable :) that's refreshing... you just made my night

comment:10 Changed 7 years ago by ystk_skm <brilliantpenguin@…>

I know that less code is fast code. Thanks for your good decision.
Take care for the same kind of claim for "isPlainObject" not to change again (and you try to revert again).

comment:11 Changed 7 years ago by timmywil

Status: reopenedassigned

comment:12 Changed 7 years ago by Rick Waldron

@timmywil, why is this reopened? (I'm just curious)

comment:13 Changed 7 years ago by timmywil

@rwaldron: Wait, I'm confused now. You reopened it. I just put it back to assigned.

comment:14 Changed 7 years ago by Rick Waldron

Resolution: wontfix
Status: assignedclosed

Yep, you're right. Oops!

comment:15 Changed 7 years ago by Rick Waldron

#13715 is a duplicate of this ticket.

comment:16 Changed 7 years ago by m_gol

Why the check:

!core_hasOwn.call(obj, "constructor")

is needed in jQuery 1.x anyway? All unit tests are passed by oldIE when this line is missing.

If it's really needed, a unit test for that should be written.

comment:17 Changed 6 years ago by Rick Waldron

#14227 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.