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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f7f80b
Simple non-breaking change to support deep extend.
prettycode Dec 17, 2014
c291c59
Check property is object before deep-extending.
prettycode Dec 17, 2014
d257ae0
Updating test to be more thorough.
prettycode Dec 17, 2014
e49ed06
Removal of delete per user caitp.
prettycode Dec 17, 2014
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
16 changes: 13 additions & 3 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,22 +333,32 @@ 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) --argsLength;

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
15 changes: 15 additions & 0 deletions test/AngularSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,21 @@ 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', age: 10, nothing: null, undef: void 0 }},
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',
age: 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not what I said before. Don't worry about the tests, I'll add some when I check this in :\

nothing: null,
undef: void 0
}
});
});
});

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