Bug Tracker

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#10867 closed enhancement (wontfix)

jQuery.extend replaces the target object when deep extend is explicitly false

Reported by: gibson042 Owned by: gibson042
Priority: low Milestone: None
Component: core Version: 1.7.1
Keywords: Cc:
Blocked by: Blocking:

Description

jQuery.extend has an optional leading parameter that toggles deep (recursive) merging. But when it is set to false, the function creates, extends, and returns a new object instead of the specified target.

Problematic call: jQuery.extend(false, objTarget, objExtension, ...)

http://jsfiddle.net/ewjuK/

Change History (13)

comment:2 Changed 10 years ago by Rick Waldron

Owner: set to gibson042
Status: newpending

Where does it say that passing false should have this behavior?

comment:3 Changed 10 years ago by gibson042

Status: pendingnew

It's implied from both documentation and code.

http://api.jquery.com/jQuery.extend/ specifies the jQuery.extend( [deep], target, object1 [, objectN] ) signature:

  • deep If true, the merge becomes recursive (aka. deep copy).
  • target The object to extend. It will receive the new properties.

And further specifies:

Keep in mind that the target object (first argument) will be modified, and will also be returned from $.extend().

Since there is a change in behavior *if* deep is true, it should be possible to specify it non-true, meaning that a jQuery.extend(false, deep3, deep4) call has a target of deep3. And since documentation specifies that the target object is modified and returned, it should further be true that jQuery.extend(false, deep3, deep4) === deep3.

I also call your attention to line 311, which tests if ( typeof target === "boolean" ); rather pointless after target = arguments[0] || {} since it might as well be if ( target === true ).

All that aside, the change makes jQuery.extend more consistent, smaller, and slightly more useful (since without it, jQuery.extend(false, deep3, deep4) is equivalent to jQuery.extend({}, deep3, deep4)).

comment:4 Changed 10 years ago by Rick Waldron

Component: unfiledcore
Priority: undecidedlow
Resolution: duplicate
Status: newclosed

Sorry, but we'e already been down this road. A quick ticket search for "extend false" brings up the original

comment:5 Changed 10 years ago by Rick Waldron

Duplicate of #9994.

comment:6 Changed 10 years ago by gibson042

With all due respect, the tickets are distinct.

#9994 was asking for the replacement of if ( typeof target === "boolean" ) with if ( target === true ).

I am asking for the replacement of target = arguments[0] || {} with target = arguments[0], to support dmethvin's $.extend(iCanHazDeep, defaults, options) example from comment 3.

comment:7 Changed 10 years ago by Rick Waldron

For clarification...

"duplicate in that they both seek to arbitrarily change jQuery.extend() where no change is needed, with regard to the left most argument"

comment:8 Changed 10 years ago by timmywil

Keywords: 1.8-discuss added
Resolution: duplicate
Status: closedreopened

rwaldron and I have discussed this and we're reopening for further discussion.

Granted passing false is not documented, but I'm of the opinion that it seems a fair assumption that false can be passed if true can be passed, especially given the possibility of a boolean var being passed. I'd rather have it do nothing special when false is passed than create a new object. In other words, these should not be equivalent, but they currently are:

$.extend(false, obj, otherObj);
$.extend({}, obj, otherObj);

It should not have an affect on the original source object that is returned.

Besides, the pull has no performance (actually a micro-optimization) or size issues so that's mostly why I'm fine with it.

comment:9 Changed 10 years ago by timmywil

Status: reopenedopen
Type: bugenhancement

comment:10 Changed 10 years ago by Erik_A

A few days before, I also ran into this issue and fixed it myself.

I replaced the declaration of target, i and depth variable declaration with the following:

deep = arguments[0] === true,
i = 1 & (deep === arguments[0]),
target = arguments[i++] || {};

What's happening in my code is the following: first the deep argument is assigned and set to true if only the first argument is actually a boolean and equals true.
The variable i is set to 1 and bitwise compared to whether or not, deep is of the same type of the fist argument. If the first argument is a boolean, then 1 & true becomes 1. (If arg0 is a object, deep === arg0 is false, thus is not a boolean. But if arg0 is false, deep === arg0 makes true).
Finally the target value is assigned: if arg0 is not a boolean, i === 0, else i === 1. After setting the target is set, i is incremented.

The deep, i and target variables containes now correct values and makes the check of target redundant. The following should be removed:

// Handle a deep copy situation
if (typeof target === "boolean") {
    deep = target;
    target = arguments[1] || {};
    // skip the boolean and the target
    i = 2;
}

comment:11 Changed 10 years ago by dmethvin

Keywords: needsdocs added; 1.8-discuss removed
Resolution: wontfix
Status: openclosed

comment:12 Changed 10 years ago by timmywil

#11411 is a duplicate of this ticket.

comment:13 Changed 9 years ago by mikesherov

Keywords: needsdocs removed
Note: See TracTickets for help on using tickets.