Bug Tracker

Opened 12 years ago

Closed 12 years ago

#1562 closed bug (fixed)

$.extend is not working properly when using deep extension

Reported by: alexo Owned by: flesler
Priority: major Milestone: 1.2.4
Component: core Version: 1.1.4
Keywords: extend, deep, bug, default Cc: alex.objelean@…
Blocked by: Blocking:

Description

When extending nested object I've got wrong results. For instance:

      var defaults = {
        css : {
          opacity: '1', position: 'relative', background: 'red'
        }
      };
      var options = {
        css : {
          opacity: '0', position: 'absolute'
        }
      };
      var settings = $.extend(true, {}, defaults, options);

Expected Result:

      settings = {
        css : {
          opacity: '0', position: 'absolute', background: 'red'
        }
      };
      defaults = {
        css : {
          opacity: '1', position: 'relative', background: 'red'
        }
      };

Actual Result:

      defaults = {
        css : {
          opacity: '0', position: 'relative', background: 'red'
        }
      };

As a result, I expect that the settings would merge defaults and options, without modifying the defaults... BUT the defaults are overridden.

Attachments (1)

extend-1562.diff (674 bytes) - added by flesler 12 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 12 years ago by hamstersoup

Here's another example:

Using deep $.extend() clobbers my source objects:

var target = {};
var s1 = { will:{name:"Will Moffat"} };
var s2 = { will:{age:29} };

$.extend(/*deep*/true,target,s1,s2);

// OK: target = { will:{name:"Will Moffat", age:29} }
// FAIL: s1 = { will:{name:"Will Moffat", age:29} }
// OK: s2 not modified

s1 has been modified, it's now the same as target.

comment:2 Changed 12 years ago by vmx

When you try the second example (comment:1) the problem is within l. 433 of jQuery 1.2:

if ( deep && typeof prop[i] == 'object' && target[i] )
	jQuery.extend( target[i], prop[i] );

After that extending argument[2] gets overwritten. I don't know why, but i found a workaround:

if ( deep && typeof prop[i] == 'object' && target[i] ) {
	var tmp = {};
	jQuery.extend( tmp, target[i], prop[i] );
	target[i] = tmp;
}

comment:3 Changed 12 years ago by wizzud

I think I've figured out why it's happening. In fn.extend()...

// Don't bring in undefined values
else if ( prop[i] != undefined )
	target[i] = prop[i];

...when prop[i] is an object - as it now can be for deep extending - the assignment is by reference, so any subsequent modifications to target[i] also get applied to prop[i].

As to a possible solution, I think that changing

// Recurse if we're merging object values
if ( deep && typeof prop[i] == 'object' && target[i] )
	jQuery.extend( target[i], prop[i] );

to

// Recurse if we're merging object values
if ( deep && typeof prop[i] == 'object' )
	target[i] = jQuery.extend( target[i] || {}, prop[i] );

might do the trick.

As a side issue regarding fn.extend(), when calling it with 'deep' set TRUE (as in the second example above), ie $.extend(true, target, s1, s2), the first pass through the major for loop always extends target into itself, because the variable a is still set to 1. I believe this is a waste of effort, and might be avoided by changing

// Handle a deep copy situation
if ( target.constructor == Boolean ) {
	deep = target;
	target = arguments[1] || {};
}

to

// Handle a deep copy situation
if ( target.constructor == Boolean ) {
	deep = target;
	target = arguments[1] || {};
	a += 1; // target has shifted by one so shift a by 1 as well
}

This also raises the question of whether jQuery can itself be extended 'deep', ie. is $.extend(true, extendJquery) valid/possible or not? If it can/is then changing

// extend jQuery itself if only one argument is passed
if ( al == 1 ) {
	target = this;
	a = 0;
}

to

// extend jQuery itself if only one argument (excluding deep) is passed
if ( al == a ) { // test arguments length against a
	target = this;
	a -= 1;
}

might also be required.

HTH

comment:4 Changed 12 years ago by davidserduke

Milestone: 1.21.2.2
Resolution: invalid
Status: newclosed

If I understand this ticket correctly, the stated problem is that extend overwrites the target when both parameters have values. This is actually as intended. The further back in the arguments list, the higher the priority. jQuery uses this function to overwrite defaults in such functions as $.ajaxSetup(). So I'm going to close this ticket. If I'm misunderstanding the writeup (which is possible since I don't see why being 'deep' matters) please reopen it with additional explaination or test cases.

Also thanks wizzud for the comment. The first target is now skipped on deep copies in [3841].

comment:5 Changed 12 years ago by hamstersoup

Resolution: invalid
Status: closedreopened

The documentation doesn't mention the 'deep' flag.

But the behaviour certainly isn't as expected. I want to be able to extend just one aspect of my defaults. For example by passing the options

{'css':{'background-color':'blue'}}

If we don't use deep extend then the whole css object will be replaced.

However when we do use deep, one of the source objects, 'defaults' is also modified, not just the target

Here's live code to demonstrate the bug. (Output is in the Firebug console)

comment:6 Changed 12 years ago by adrien.gibra

I've tested a little hack in jQuery 1.2.3

The actual code recurse just one level deeper... it sounds more logical to really recurse. And to avoid 'defaults' to be modified I use an empty object as target.

Replace:

// Recurse if we're merging object values
if ( deep && options[ name ] && typeof options[ name ] == "object" && target[ name ] && !options[ name ].nodeType )
	target[ name ] = jQuery.extend( target[ name ], options[ name ] );

by:

// Recurse if we're merging object values
if ( deep && options[ name ] && typeof options[ name ] == "object" && target[ name ] && !options[ name ].nodeType )
	target[ name ] = jQuery.extend( true, {}, target[ name ], options[ name ] );

This seems to do the trick, but i still wonder why

target[ name ] = jQuery.extend( true, {}, target[ name ], options[ name ] );

is not equal to

jQuery.extend( true, target[ name ], options[ name ] );

In my understanding this should do exactly the same thing, but the second piece of code will modify the 'defaults'

comment:7 Changed 12 years ago by flesler

Resolution: fixed
Status: reopenedclosed

This was applied some time ago.

comment:8 Changed 12 years ago by flesler

Milestone: 1.2.21.2.4

comment:9 in reply to:  7 Changed 12 years ago by hamstersoup

Resolution: fixed
Status: closedreopened

Replying to flesler:

This was applied some time ago.

The test I created still seems to be failing: http://mqlx.com/~willmoffat/learn_feature/jquery/deep_extend.html

Either I misunderstand how 'deep' is supposed to work or this bug should still be open.

comment:10 Changed 12 years ago by flesler

Yes sorry, I had this ticket opened and mistakenly closed it while doing mass closing (related tickets).

I'll try to fix this ASAP.

comment:11 Changed 12 years ago by flesler

Ok, I think this is it, this patch makes a test fail, but I think the test needs to be changed. I still need to try it some more.

Changed 12 years ago by flesler

Attachment: extend-1562.diff added

comment:12 Changed 12 years ago by flesler

Owner: set to flesler
Status: reopenednew

comment:13 Changed 12 years ago by flesler

Resolution: fixed
Status: newclosed

Fixed at [5599].

Note: See TracTickets for help on using tickets.