New "clone" behavior

[RebeccaStevens]

Overview

1. No unnecessary cloning - no cloning by default.
2. Un-deprecate the "clone" flag or introduce a new flag (maybe called "deepClone") that when true, will perform the current default behavior.
3. Introduce a new flag called "mergeWithTarget" that when true, will result in the target (first parameter) being mutated as a result of the merge. No value is returned when operating in this mode.

Details

1. No unnecessary cloning

The library is called "deep merge" not "deep merge and clone" so it doesn't make sense to deep clone out of the box by default. Cloning adds a lot of overhead and most of the time isn't needed.

When two objects are being merged, neither object should be mutated. A new object should be created to be the merged value. When merging only 1 object (with undefined or some other non-mergable type), that object is simply supplied as the merged value.

re: #163

2. Still allow cloning

Cloning while merging is still something that is definitely valid and there should be an option to perform this behavior.

3. Allow in-place merging

Some times a new object isn't wanted at all; instead a changes to an object want to me merged in place.

re: #186

There is already an open PR that adds this behavior ~to the "clone" flag~. #209 ~But the issue with having this behavior on the "clone" flag is that it that it is valid to want to both "clone" and not mutate the any of the parameters.~

PS

@TehShrike I can implementing feature 1 and 2 if you like. ~@creativeux Do you think you could update #209 to put the in-place merging behind a new "mergeWithTarget" flag?~ [done]

I'll also be able to implement these changes into the typings I'm working on in #211.

PPS

@TehShrike While working on #211 I noticed that structure of this project is a bit out of date. Would you like me to also work on modernizing it?

Proposal of things of the top of my head:

• Add eslint
• Refactor code into multiple files - these files will live in a "src/" directory.
• Update rollup and other dev dependencies
• Switch to using TypeScript

[creativeux]

@RebeccaStevens I can make that change to #209. It might be a few days, but I'll add it to my list.

[TehShrike]

My personal opinion is that by default, libraries should not mutate any of their inputs.

I see the "merge" part of the name as referring to the properties of the inputs, not their values – e.g. merging { a: true } and { b: false } gets you an object with both the a and b properties on it.

I've never been comfortable with the inconsistency where in "mutation mode" (clone=false), deepmerge would mutate every object except for the target object itself. Does that not seem inconsistent to you?

---

I do have an eslint config locally, and I use it for autoformatting – I usually avoid pushing less-meaningful style rules onto contributors, and autoformat before publishing when necessary :-x

I don't have strong opinions about refactoring into multiple files. ~100 lines isn't too bad, and the call stack never gets very deep in deepmerge's internals, so there's not much potential to hide some functions behind a new module.

Updating dev dependencies would be much appreciated.

Moving to TypeScript would be cool. I'm afraid the changes in the v5 branch are going to languish away forever though :-(

[RebeccaStevens]

My personal opinion is that by default, libraries should not mutate any of their inputs.

I strongly agree with this. mergeWithTarget (coming from #209) should definitely be false by default.

I've never been comfortable with the inconsistency where in "mutation mode" (clone=false), deepmerge would mutate every object except for the target object itself. Does that not seem inconsistent to you?

Yeah, that is inconsistent. I guess I was only thinking about one particular use case with my first point (that use case being when merging an object with undefined). I'll reword point one to handle all use cases.

I do have an eslint config locally, and I use it for autoformatting – I usually avoid pushing less-meaningful style rules onto contributors, and autoformat before publishing when necessary :-x

I guess that's a good approach for newer people. I just got a little confused when I was making my changes to the types and test files in #211, not knowing how to format things properly. I like to auto format things to so it would be nice if the same autoformatter you're using was available.

Moving to TypeScript would be cool. I'm afraid the changes in the v5 branch are going to languish away forever though :-(

I'll work off that branch so the changes don't get lost. I see that branch has fallen a bit behind master; could you rebase it?

[TehShrike]

Rebasing got a bit gnarly, so I merged master into v5.

---

mergeWithTarget (coming from #209) should definitely be false by default

This doesn't address my issue with the first point though –

const initial_foo = { funky: 'maaaaaybe' }

const result = deepmerge({ foo: initial_foo }, { foo: { something_else: true } })

if foo is mutated, the library has mutated its inputs, and I'm not a fan.

I would prefer to leave the clone value, but remove the inconsistency where when clone is false, everything can be mutated except the root. It is technically a breaking change and would result in a major version bump, but I think it's worth it to remove what seems like surprising/unexpected behavior.

[RebeccaStevens]

Sorry, my last reply wasn't clear.

when clone is false, everything can be mutated except the root

I agree that this is unwanted behavior and it should be removed.

I updated the original description to no longer say "Make the current behavior when clone is false the default behavior" but instead say "No unnecessary cloning" (which is what I was assuming clone === false actually did - it fooled me).

Here's an example of what I mean by this (new expected behavior):

const initial_foo = { a: 1 }
const initial_bar = { b: 2 }

const result = deepmerge({ foo: initial_foo, bar: initial_bar }, { foo: { c: true } })

result.foo === initial_foo // false - cloning was necessary.
result.bar === initial_bar // true  - cloning was unnecessary.