Opened 15 years ago
Closed 15 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: | [email protected]… |
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 (14)
comment:1 Changed 15 years ago by
comment:2 Changed 15 years ago by
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 15 years ago by
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 15 years ago by
Milestone: | 1.2 → 1.2.2 |
---|---|
Resolution: | → invalid |
Status: | new → closed |
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 15 years ago by
Resolution: | invalid |
---|---|
Status: | closed → reopened |
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 15 years ago by
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 follow-up: 9 Changed 15 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
This was applied some time ago.
comment:8 Changed 15 years ago by
Milestone: | 1.2.2 → 1.2.4 |
---|
comment:9 Changed 15 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 15 years ago by
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 15 years ago by
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 15 years ago by
Attachment: | extend-1562.diff added |
---|
comment:12 Changed 15 years ago by
Owner: | set to flesler |
---|---|
Status: | reopened → new |
Here's another example:
Using deep $.extend() clobbers my source objects:
s1 has been modified, it's now the same as target.