Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(angular.extend) - Simple non-breaking change to support deep extend #10507

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -333,22 +333,34 @@ function setHashKey(obj, h) {
* Extends the destination object `dst` by copying own enumerable properties from the `src` object(s)
* to `dst`. You can specify multiple `src` objects. If you want to preserve original objects, you can do so
* by passing an empty object as the target: `var object = angular.extend({}, object1, object2)`.
* Note: Keep in mind that `angular.extend` does not support recursive merge (deep copy).
* Note: Pass `true` in as the last argument to perform a recursive merge (deep copy).
*
* @param {Object} dst Destination object.
* @param {...Object} src Source object(s).
* @returns {Object} Reference to `dst`.
*/
function extend(dst) {
var h = dst.$$hashKey;
var argsLength = arguments.length;
var isDeep = (argsLength >= 3) && (arguments[argsLength - 1] === true);

for (var i = 1, ii = arguments.length; i < ii; i++) {
if (isDeep) {
delete arguments[argsLength - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

declare ii before the loop, decrement it if isDeep

Copy link
Contributor

Choose a reason for hiding this comment

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

in fact you know what, argsLength is declared already, so just use that instead :)

Copy link
Contributor

Choose a reason for hiding this comment

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

if (isDeep) --argsLength;

We don't need to delete anything, it's slow, and deleting the argument doesn't really do anything meaningful (it doesn't change the length value). I said this in like the first 3 comments =)

}

for (var i = 1; i < argsLength; i++) {
var obj = arguments[i];
if (obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You misunderstood me in that last commit --- I mean on line 353, the condition should not be if (obj), it should be if (isObject(obj) || isFunction(obj))

Copy link
Contributor

Choose a reason for hiding this comment

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

Make this change, please.

var keys = Object.keys(obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

That said, I really think we should only be taking this path if isObject(obj) || isFunction(obj) --- it doesn't really make sense otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

For instance:

Object.keys(4); // will return the own-enumerable-property-keys of `new Number(4);` --- that's not very useful! It won't have any own properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in ES5 it will throw for all non-objects, which is not very useful! So WebKit/JSC would throw here anyways. Newer browsers will take the ES6 path and convert the primitive ToObject, and do the wrong stuff with it.

for (var j = 0, jj = keys.length; j < jj; j++) {
var key = keys[j];
dst[key] = obj[key];
var src = obj[key];

if (isDeep && isObject(dst[key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If dst[key] is anything other than an object, we really need to replace it with an object --- I think copy() will use an Array if the source is an array, too.

src = extend(dst[key], src, true);
}

dst[key] = src;
Copy link
Contributor

Choose a reason for hiding this comment

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

we just copied everything from src into dst[key] --- why are we clobbering dst[key] now? Should only do this if src is a non-object

}
}
}
Expand Down
12 changes: 12 additions & 0 deletions test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,18 @@ describe('angular', function() {
// make sure we retain the old key
expect(hashKey(dst)).toEqual(h);
});

it('should perform deep extend when last argument is true', function() {
var src = { foo: { bar: 'foobar' }},
dst = { foo: { bazz: 'foobazz' }};
extend(dst, src, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

add a test case where dst has a primitive (non-falsy) foo property --- also a test where it has a null foo property

Copy link
Contributor

Choose a reason for hiding this comment

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

also add some tests where properties have null and undefined values :>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the tests I've been asking for, and address all of the comments I've brought up --- then we can land this.

You can avoid the complicated squash in the future by just doing git commit --amend rather than adding new commits

expect(dst).toEqual({
foo: {
bar: 'foobar',
bazz: 'foobazz'
}
});
});
});

describe('shallow copy', function() {
Expand Down