Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Make sure to do a deep copy on arrays. #5750
  • Loading branch information
fortes authored and jeresig committed Jan 6, 2010
1 parent 6861b5d commit 0d1a2c1
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 6 deletions.
8 changes: 4 additions & 4 deletions src/core.js
Expand Up @@ -316,10 +316,10 @@ jQuery.extend = jQuery.fn.extend = function() {
continue;
}

// Recurse if we're merging object literal values
if ( deep && copy && jQuery.isPlainObject(copy) ) {
// Don't extend not object literals
var clone = src && jQuery.isPlainObject(src) ? src : {};
// Recurse if we're merging object literal values or arrays
if ( deep && copy && ( jQuery.isPlainObject(copy) || jQuery.isArray(copy) ) ) {
var clone = src && ( jQuery.isPlainObject(src) || jQuery.isArray(src) ) ? src
: jQuery.isArray(copy) ? [] : {};

// Never move original objects, clone them
target[ name ] = jQuery.extend( deep, clone, copy );
Expand Down
9 changes: 7 additions & 2 deletions test/unit/core.js
Expand Up @@ -643,7 +643,7 @@ test("jQuery.merge()", function() {
});

test("jQuery.extend(Object, Object)", function() {
expect(25);
expect(27);

var settings = { xnumber1: 5, xnumber2: 7, xstring1: "peter", xstring2: "pan" },
options = { xnumber2: 1, xstring2: "x", xxx: "newstring" },
Expand All @@ -653,7 +653,9 @@ test("jQuery.extend(Object, Object)", function() {
deep1copy = { foo: { bar: true } },
deep2 = { foo: { baz: true }, foo2: document },
deep2copy = { foo: { baz: true }, foo2: document },
deepmerged = { foo: { bar: true, baz: true }, foo2: document };
deepmerged = { foo: { bar: true, baz: true }, foo2: document },
arr = [1, 2, 3],
nestedarray = { arr: arr };

jQuery.extend(settings, options);
same( settings, merged, "Check if extended: settings must be extended" );
Expand All @@ -668,6 +670,9 @@ test("jQuery.extend(Object, Object)", function() {
same( deep2.foo, deep2copy.foo, "Check if not deep2: options must not be modified" );
equals( deep1.foo2, document, "Make sure that a deep clone was not attempted on the document" );

ok( jQuery.extend(true, [], arr) !== arr, "Deep extend of array must clone array" );
ok( jQuery.extend(true, {}, nestedarray).arr !== arr, "Deep extend of object must clone child array" );

var empty = {};
var optionsWithLength = { foo: { length: -1 } };
jQuery.extend(true, empty, optionsWithLength);
Expand Down

3 comments on commit 0d1a2c1

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 0d1a2c1 Jan 13, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by the intention/implementation of this commit.

  1. Iterating Arrays with for-in loops can produce different behaviors in older browsers. Is really length not-enumerable in every supported browser?.

  2. If src is an array, and copy is a plain object, then the current implementation will do extend(true, src, copy) instead of extend(true, {}, copy).
    Vice versa, if src is a plain object, and copy is an array, then the current implementation will do extend(true, src, copy) instead of extend(true, [], copy)

  3. The test case at line 673 have no much sense - it do not evolving any deep extending.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 0d1a2c1 Jan 13, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I the only one perceiving problems here?

@GarrettS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic.

The analysis you provided appears to be the intent of the author.

That extend method has no overview, comments, no formal parameters, is long, recursive, accepts variable number of optional "options" arguments and accepts an optional deep argument as the first argument.

Extra effort has gone into writing confusing, complicated code.

The first optional argument deep requires the function to perform conditional runtime behavior.

Instead, the extend function should use formal parameters. This will help define what it does and will allow optimizing implementation to optimize the call when arguments is not in the function body.

The boolean flag "deep" as an optional first argument is another complication. The deep parameter should acceptable as an optional last argument.

Identifier options is the supplier of property values to copy to target. options seem like a misleading name. It is not "options" but a supplier of properties to target.

Please sign in to comment.