Add option to clone objects before merge. Fixes #28 issue. #44
Conversation
There was a problem hiding this comment.
Looks pretty reasonable to me - I want to make sure we don't miss some case where we might be copying objects over though, so some tests with dates and arrays, and also some identical-looking objects would be good.
It might be simpler to assert src.a !== target.a and src.a.key !== target.a.key to assert that cloning was happening. since that's what I think we care about most - if it's not a primitive, we need to make sure that the value is a copy, and not the same object in memory.
| @@ -31,15 +31,23 @@ function defaultArrayMerge(target, source, optionsArgument) { | |||
| } | |||
|
|
|||
| function mergeObject(target, source, optionsArgument) { | |||
| var clone = optionsArgument && optionsArgument.hasOwnProperty('clone') | |||
There was a problem hiding this comment.
var clone = optionsArgument && optionsArgument.close === true is probably safe enough, since we're fine with defaulting to false.
| merged.a.d = "bar-modifies" | ||
| t.equal(src.b.c, "foo") | ||
| t.equal(target.a.d, "bar") | ||
|
|
There was a problem hiding this comment.
These two tests should probably assert that merged deep equals expected, since the two "modified" values wouldn't conflict with each other.
| var destination = {} | ||
| if (isMergeableObject(target)) { | ||
| Object.keys(target).forEach(function (key) { | ||
| destination[key] = target[key] | ||
| var val = target[key] | ||
| if (clone && typeof val === 'object') |
There was a problem hiding this comment.
I'm guessing that this should be isMergeableObject(val) - but it would be good to have some tests covering dates/arrays first to make sure.
|
Think you'll get a chance to revisit this @gagan-bansal? I'm fine leaving this pull request open for a while if you're just waiting for the time to get back to it. |
|
@TehShrike I read your comments and all points are fine and need to be implemented. Because of time I could not do take up this. This weekend (ie today and tomorrow) I'll implement the required changes. |
|
No worries. Whenever you get the chance. 👍 |
|
I have rechecked and corrected as per the suggestion. Thanks for guidance. |
| var src = { key1: /efg/ } | ||
| var expected = { key1: /efg/ } | ||
|
|
||
| t.deepEqual(merge(target, src, {clone: true}), expected) |
There was a problem hiding this comment.
deepEqual won't assert as much as we want, here - if this test is meant to assert that the original regex object is copied to the new object, we need to assert t.equal(target.key1, src.key1) (the expected object isn't actually what we expect, since we should expect the regular expression object to be copied directly instead of just being made up of the same pattern).
| var actual = merge(target, source, {clone: true}) | ||
|
|
||
| t.deepEqual(actual, expected) | ||
| t.equal(actual.key.valueOf(), tuesday.valueOf()) |
There was a problem hiding this comment.
Similar to the regex test - we need to assert that the objects are the same object in memory, not that their underlying values are the same. t.equal(target.key, tuesday) (no expected object needed).
| var merged = merge(target, src, {clone: true}) | ||
| t.deepEqual(merged, expected) | ||
| t.ok(Array.isArray(merge(target, src).key1)) | ||
| t.ok(Array.isArray(merge(target, src).key2)) |
There was a problem hiding this comment.
I don't think these three assertions (deepEqual, and the two isArrays) are necessary for this test, we should have other tests covering that behavior. The cloning assertions below are all we should be asserting here.
| @@ -18,28 +18,44 @@ function isMergeableObject(val) { | |||
|
|
|||
| function defaultArrayMerge(target, source, optionsArgument) { | |||
| var destination = target.slice() | |||
| var clone = optionsArgument && optionsArgument.clone === true | |||
| source.forEach(function(e, i) { | |||
| if (typeof destination[i] === 'undefined') { | |||
| destination[i] = e | |||
There was a problem hiding this comment.
I think e needs to be cloned in this case, too - I think a failing test for this case might be
var a = { key: 'yup' }
var target = []
var source = [a]
var output = deepmerge(target, source)
t.notEqual(output[0], a)
t.equal(output[0].key, 'yup')But I haven't tested that to make sure it fails right now.
There was a problem hiding this comment.
@TehShrike Good analytical thinking.
|
Improved the test cases. |
|
Looks good to me! |
|
Note to self: check after merging, but I don't think that |
|
@TehShrike Yes // current
return Array.isArray(target) ? arrayMerge(target, source, optionsArgument) :
source.slice()
// can be
if (clone) source = deepmerge([], source)
return Array.isArray(target) ? arrayMerge(target, source, optionsArgument) : source |
|
I opened a pull request with a fix at #48. I'll push this out as a feature release once it gets merged. |
The issue was noted in #44 (comment)
|
Published as 1.2.0 |
The issue was noted in TehShrike/deepmerge#44 (comment)
Added option
cloneas boolean to clone objects before merge. Fixes #28 issue.