Skip to main content

Bug Tracker

Side navigation

#1562 closed bug (fixed)

Opened September 04, 2007 09:00AM UTC

Closed May 14, 2008 07:46PM UTC

$.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@isdc.ro
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)
Change History (13)

Changed September 15, 2007 02:41AM UTC by hamstersoup comment:1

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.

Changed September 15, 2007 02:14PM UTC by vmx comment:2

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

Changed September 16, 2007 01:04PM UTC by wizzud comment:3

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

Changed December 04, 2007 09:23AM UTC by davidserduke comment:4

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

Changed February 06, 2008 04:46PM UTC by hamstersoup comment:5

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)

Changed April 16, 2008 11:47AM UTC by adrien.gibra comment:6

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'

Changed May 11, 2008 10:50PM UTC by flesler comment:7

resolution: → fixed
status: reopenedclosed

This was applied some time ago.

Changed May 13, 2008 05:31PM UTC by flesler comment:8

milestone: 1.2.21.2.4

Changed May 13, 2008 08:51PM UTC by hamstersoup comment:9

resolution: fixed
status: closedreopened

Replying to [comment:7 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.

Changed May 13, 2008 09:13PM UTC by flesler comment:10

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

I'll try to fix this ASAP.

Changed May 13, 2008 10:55PM UTC by flesler comment:11

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 May 13, 2008 10:56PM UTC by flesler comment:12

owner: → flesler
status: reopenednew

Changed May 14, 2008 07:46PM UTC by flesler comment:13

resolution: → fixed
status: newclosed

Fixed at [5599].