$.extend is not working properly when using deep extension

[alexo]

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.

[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.

[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;
}

[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

[davidserduke]

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].

[hamstersoup]

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)

[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'

[flesler]

This was applied some time ago.

[hamstersoup]

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.

[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.

[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.

[flesler]

Fixed at [5599].