Add "clone" option

[gagan-bansal]

here is the test

var src = {
  "b": {
    "c": 'foo'
  }
}

var target = {
  "a": {}
}

var expected = {
  "a": {},
  "b": {
    "c": 'foo'
  }
}
var combined = merge(target, src)
src.b.c = 'bar'

console.log(combined.b.c)
// output as
// 'bar'
// though it should be 'foo'

Is this intentional behavior? I think in most of the cases source object should be cloned.

[TehShrike]

Interesting. So the issue is that you (quite reasonably) expected that all values would be recursively copied, and it definitely doesn't do that.

The docs don't claim that it recursively copies, but it sure does look like it would. I'm tempted to add this behavior because it's what I'm more likely to want, but I'm averse to adding it because it would be a breaking change.

In any case, if the change doesn't get added and #29 isn't merged, the documentation should at least be made clearer on this behavior.

Any other opinions? Is recursive copying worth the breaking change?

[gagan-bansal]

Yes it will break the present behavior and sure its major change.

But as the document states that a new merged object would be created then expectations are that the changes in new object would not affect the original objects. If someone wants the current behavior (without cloning) then generally extend is used.

As the change is major I think more suggestions are required.

[gagan-bansal]

Passing clone (boolean) as an argument would be better. Like

merge(x, y, clone)

and default value for clone should be false.

[TehShrike]

Yeah, that would probably be the way to go. #37 adds an options object - a clone property on that would be a good way to enable this behavior.

[gagan-bansal]

OK once you merge #37 I'll make changes to #29 so that clone as option is passed in third argument options.

[TehShrike]

👍

[TehShrike]

Published as 1.2.0