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

Commit

Permalink
feat(angular.merge): provide an alternative to angular.extend that …
Browse files Browse the repository at this point in the history
…merges 'deeply'

Closes #10507
Closes #10519
  • Loading branch information
caitp authored and petebacondarwin committed Mar 3, 2015
1 parent f591776 commit c0498d4
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 14 deletions.
1 change: 1 addition & 0 deletions src/.jshintrc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"extend": false,
"toInt": false,
"inherit": false,
"merge": false,
"noop": false,
"identity": false,
"valueFn": false,
Expand Down
69 changes: 55 additions & 14 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
extend: true,
toInt: true,
inherit: true,
merge: true,
noop: true,
identity: true,
valueFn: true,
Expand Down Expand Up @@ -318,6 +319,31 @@ function setHashKey(obj, h) {
}
}


function baseExtend(dst, objs, deep) {
var h = dst.$$hashKey;

for (var i = 0, ii = objs.length; i < ii; ++i) {
var obj = objs[i];
if (!isObject(obj) && !isFunction(obj)) continue;
var keys = Object.keys(obj);
for (var j = 0, jj = keys.length; j < jj; j++) {
var key = keys[j];
var src = obj[key];

if (deep && isObject(src)) {
if (!isObject(dst[key])) dst[key] = isArray(src) ? [] : {};
baseExtend(dst[key], [src], true);
} else {
dst[key] = src;
}
}
}

setHashKey(dst, h);
return dst;
}

/**
* @ngdoc function
* @name angular.extend
Expand All @@ -328,30 +354,45 @@ 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:** Keep in mind that `angular.extend` does not support recursive merge (deep copy). Use
* {@link angular.merge} for this.
*
* @param {Object} dst Destination object.
* @param {...Object} src Source object(s).
* @param {boolean=} deep if the last parameter is set to `true`, objects are recursively merged

This comment has been minimized.

Copy link
@gkalpak

gkalpak Mar 3, 2015

Member

This parameter does not exist (any more).

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Mar 4, 2015

Member

Right

* (deep copy). Defaults to `false`.
* @returns {Object} Reference to `dst`.
*/
function extend(dst) {
var h = dst.$$hashKey;
return baseExtend(dst, slice.call(arguments, 1), false);
}

for (var i = 1, ii = arguments.length; i < ii; i++) {
var obj = arguments[i];
if (obj) {
var keys = Object.keys(obj);
for (var j = 0, jj = keys.length; j < jj; j++) {
var key = keys[j];
dst[key] = obj[key];
}
}
}

setHashKey(dst, h);
return dst;
/**
* @ngdoc function
* @name angular.merge
* @module ng
* @kind function
*
* @description
* Deeply 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.merge({}, object1, object2)`.
*
* Unlike {@link angular.extend extend()}, `merge()` recursively descends into object properties of source
* objects, performing a deep copy.
*
* @param {Object} dst Destination object.
* @param {...Object} src Source object(s).
* @returns {Object} Reference to `dst`.
*/
function merge(dst) {
return baseExtend(dst, slice.call(arguments, 1), true);
}



function toInt(str) {
return parseInt(str, 10);
}
Expand Down
1 change: 1 addition & 0 deletions src/AngularPublic.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ function publishExternalAPI(angular) {
'bootstrap': bootstrap,
'copy': copy,
'extend': extend,
'merge': merge,
'equals': equals,
'element': jqLite,
'forEach': forEach,
Expand Down
1 change: 1 addition & 0 deletions test/.jshintrc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"nextUid": false,
"setHashKey": false,
"extend": false,
"merge": false,
"toInt": false,
"inherit": false,
"noop": false,
Expand Down
70 changes: 70 additions & 0 deletions test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ describe('angular', function() {
expect(hashKey(dst)).not.toEqual(hashKey(src));
});


it('should retain the previous $$hashKey', function() {
var src,dst,h;
src = {};
Expand All @@ -395,6 +396,7 @@ describe('angular', function() {
expect(hashKey(dst)).toEqual(h);
});


it('should work when extending with itself', function() {
var src,dst,h;
dst = src = {};
Expand All @@ -405,6 +407,74 @@ describe('angular', function() {
});
});


describe('merge', function() {
it('should recursively copy objects into dst from left to right', function() {
var dst = { foo: { bar: 'foobar' }};
var src1 = { foo: { bazz: 'foobazz' }};
var src2 = { foo: { bozz: 'foobozz' }};
merge(dst, src1, src2);
expect(dst).toEqual({
foo: {
bar: 'foobar',
bazz: 'foobazz',
bozz: 'foobozz'
}
});
});


it('should replace primitives with objects', function() {
var dst = { foo: "bloop" };
var src = { foo: { bar: { baz: "bloop" }}};
merge(dst, src);
expect(dst).toEqual({
foo: {
bar: {
baz: "bloop"
}
}
});
});


it('should replace null values in destination with objects', function() {
var dst = { foo: null };
var src = { foo: { bar: { baz: "bloop" }}};
merge(dst, src);
expect(dst).toEqual({
foo: {
bar: {
baz: "bloop"
}
}
});
});


it('should copy references to functions by value rather than merging', function() {
function fn() {}
var dst = { foo: 1 };
var src = { foo: fn };
merge(dst, src);
expect(dst).toEqual({
foo: fn
});
});


it('should create a new array if destination property is a non-object and source property is an array', function() {
var dst = { foo: NaN };
var src = { foo: [1,2,3] };
merge(dst, src);
expect(dst).toEqual({
foo: [1,2,3]
});
expect(dst.foo).not.toBe(src.foo);
});
});


describe('shallow copy', function() {
it('should make a copy', function() {
var original = {key:{}};
Expand Down

6 comments on commit c0498d4

@Narretz
Copy link
Contributor

@Narretz Narretz commented on c0498d4 Mar 3, 2015

Choose a reason for hiding this comment

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

I might be wrong, but it seems like this feature is not used in core. So is this just a convenience function? But aren't we telling people quite often that they shouldn't rely on these helper functions?

@gkalpak
Copy link
Member

@gkalpak gkalpak commented on c0498d4 Mar 3, 2015

Choose a reason for hiding this comment

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

As discussed in #10507, this was originally supposed to be a minor and harmless enhancement of extend(), but evolved into a separate function 😃

@Narretz, has a point here.
(Plus, the names aren't super-intuitive imo.)

@petebacondarwin
Copy link
Member

Choose a reason for hiding this comment

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

It's a minimal code change (with no breaking change), which happened to work better with a new method name rather than a less intuitive extra parameter. It is still harmless and adds only a few bytes.

@gkalpak
Copy link
Member

@gkalpak gkalpak commented on c0498d4 Mar 5, 2015

Choose a reason for hiding this comment

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

I can think of a dozen "harmless" utility functions, that only add a few bytes each, do not introcude a breaking change, are not used by the core and are potentially useful to someone.
The point is, it is contradictory to say "exposing these helper functions was a bad idea to start with, we shouldn't have done that, now let's add one more (that's not even used by the core; just in case someone needs it)".

I don't feel strongly about it, just confused 😕

@petebacondarwin
Copy link
Member

Choose a reason for hiding this comment

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

Sorry. This sneaked in by presenting itself as a tiny harmless change.

@gkalpak
Copy link
Member

@gkalpak gkalpak commented on c0498d4 Mar 5, 2015

Choose a reason for hiding this comment

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

😃

Please sign in to comment.