Bug Tracker

Opened 5 years ago

Closed 5 years ago

Last modified 4 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:


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, ...)


Change History (13)

comment:2 Changed 5 years ago by rwaldron

  • Owner set to gibson042
  • Status changed from new to pending

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

comment:3 Changed 5 years ago by gibson042

  • Status changed from pending to new

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 5 years ago by rwaldron

  • Component changed from unfiled to core
  • Priority changed from undecided to low
  • Resolution set to duplicate
  • Status changed from new to closed

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

comment:5 Changed 5 years ago by rwaldron

Duplicate of #9994.

comment:6 Changed 5 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 5 years ago by rwaldron

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 5 years ago by timmywil

  • Keywords 1.8-discuss added
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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 5 years ago by timmywil

  • Status changed from reopened to open
  • Type changed from bug to enhancement

comment:10 Changed 5 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 5 years ago by dmethvin

  • Keywords needsdocs added; 1.8-discuss removed
  • Resolution set to wontfix
  • Status changed from open to closed

comment:12 Changed 5 years ago by timmywil

#11411 is a duplicate of this ticket.

comment:13 Changed 4 years ago by mikesherov

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