feat(collections): add deepMerge

[lowlighter]

This implements deepMerge, which merges deeply two records together into the first one.

deepMerge({           // {
  foo: {              //   foo: {
    bar: true,        //     bar: true,
  },                  //     baz: true,
}, {                  //     quux: {},
  foo: {              //   },
    baz: true,        //   qux: true,
    quux: {},         // }
  },
  qux: true,
})

For convenience, a few options are proposed to handle edge cases:

• arrays, sets and maps can change their merging strategy from "replace" to "concat". In the latter mode, values from both records will be kept
• includeNonEnumerable can be used to include properties and symbols which have an hidden visibility

Additional implementation notes:

• All PropertyKey types are supported (including symbol)
• Built-ins (like RegExp, Date, ...) and user classes won't be treated as objects, so the recursive merging won't apply on them (instead their reference is just replaced in target record)

Ref: #1065

---

• Typings
  • Basic typing
  • Improved maps, arrays, sets typing
  • Probably missing a recursive typing somewhere
  • Handle merging strategy to reflect correct typing
• Add signature for Partial<T> : T
• Change default to merge instead
• Add example to comment and README.md
• Rename typing in PascalCase instead and export the options one
• First clone the target object to avoid mutating in place
• Improve prototype check

[kt3k]

Tried on my end. Even typing seems working recursively with many cases. Great work!

But I found some cases where the typing doesn't match to the result, for example:

const a = { a: 1 };
const b = { a: "a" };
const c = deepMerge(a, b);

here c.a is "a", but it has never type. Can this be addressed somehow? If not, maybe better to note something about it in the document

[LionC]

Thanks for implementing this! I am really excited about a standardized deepMerge :-)

Some notes:

1. I think you could define recursive types yourself to actually have an outcome where the type of the right side overrides the one on the left. I think we should do this, as it would solve the problem pointed out by @kt3k . We should also be aware that concat will change the type of the resulting Array / Map etc.
2. The node deep-merge module offers an overloaded signature with a type parameter, allowing to specify the output and requiring the arguments to be partials deepMerge<T extends SomeRecord>(a: Partial<T>, b: Partial<T>): T. I would propose adding this, as this allows people to tell the compiler about their intent and get it to give them errors if they make a mistake.
3. I am not sure that "replace" is the default (meaning is more often used), in my experience IF you have collections on the same property name, you want to have all of them. Might be personal though.
4. Some more code notes but I will add them in a review

[lowlighter]

But I found some cases where the typing doesn't match to the result, for example:

const a = { a: 1 };
const b = { a: "a" };
const c = deepMerge(a, b);

here c.a is "a", but it has never type. Can this be addressed somehow? If not, maybe better to note something about it in the document
(@kt3k )

I think you could define recursive types yourself to actually have an outcome where the type of the right side overrides the one on the left. I think we should do this, as it would solve the problem pointed out by @kt3k . We should also be aware that concat will change the type of the resulting Array / Map etc.
(@LionC)

I'll try to investigate to solve these typing issues if possible 👍

It's possible that it comes from if (isMergeable<T | U>(a) && isMergeable<T | U>(b)) and we should just let TypeScript infers the typing but didn't test yet

But if you have any leads on how to fix this, I'll take them 🙂

Thanks for taking time to review and advise on this feature

[lowlighter]

(see comment below)

[lowlighter]

deepMerge typing is now fully functionning and should cover most cases from what I tested (see examples below).
It also handles the merging strategy, so if set to merge, you'll get a data structure with union typing inside, and if set to replace you'll get the right-operand data structure typing 👍

User code	Typing (infered)
const a = deepMerge({ foo:true, baz:{ qux:NaN }, quux:{ grault:"test" } }, { bar:"string", baz:{ corge:true }, quux:"ok" })	const a: { foo: boolean; bar: string; quux: string; baz: { qux: number; corge: boolean; }; }
const a = deepMerge({ foo:{ bar:new Set([1, 2, 3]), baz:[1, 2, 3], qux:new Map([["a", 1], ["b", 2]]) } }, { foo:{ bar:new Set(["a", "b", "c"]), baz:["a", "b", "c"], qux:new Map([["c", true]]) } })	const a: { foo: { bar: Set<string | number>; baz: (string | number)[]; qux: Map<string, number | boolean>; }; }
const a = deepMerge({ foo:{ bar:new Set([1, 2, 3]), baz:[1, 2, 3], qux:new Map([["a", 1], ["b", 2]]) } }, { foo:{ bar:new Set(["a", "b", "c"]), baz:["a", "b", "c"], qux:new Map([["c", true]]) } }, {sets:"replace", arrays:"replace", maps:"replace"})	const a: { foo: { bar: Set<string>; baz: string[]; qux: Map<string, boolean>; }; }

There are almost more typing logic than the actual algorithm but result seems worth it 🙂

I think this is ready for review, I don't have anything to add and this seems goods to me for a first iteration

[LionC]

deepMerge typing is now fully functionning and should cover most cases from what I tested (see examples below).

It also handles the merging strategy, so if set to merge, you'll get a data structure with union typing inside, and if set to replace you'll get the right-operand data structure typing 👍

There are almost more typing logic than the actual algorithm but result seems worth it 🙂

I think this is ready for review, I don't have anything to add and this seems goods to me for a first iteration

Awesome! I will take a look tonight. Looking forward to it :-)

[kt3k]

Great explanation! 👏

[LionC]

Me and @lowlighter are in contact on Discord to finish this @kt3k , this will likely be ready for final review tomorrow.

[LionC]

@kt3k we have decided to remove nonEnumerable props and made the code a bit more efficient regarding runtime, memory and readability. This should be good enough for a first releasable version for now - we thought about adding more generalized merging options, but decided to move them to a PR in the future.

TL;DR: This is ready to be merged^^

[timreichen]

Instead of naming it deepMerge, how about deepAssign as in Object.assign?

[lowlighter]

Instead of naming it deepMerge, how about deepAssign as in Object.assign?

It doesn't mutate the left operand like assign though. A new object is created instead

[timreichen]

@lowlighter I think it would be a good idea to have it function more like the native shallow version aka assign, in a sense that it should mutate a target object and merge n objects instead of just two. That makes the function more flexible.

const mutatedA = deepAssign(a, b, c)

To create a new object

const d = deepAssign({}, a, b, c)

[lowlighter]

Merging inplace was the original the design, but I've been told that std/collections' philosophy is to never mutate inplace and instead return a new object instead.

As for variable arguments I'm not against but it would make it awkward to pass options I think

[kt3k]

@timreichen I think deepAssign is worth another proposal. Let's continue discussion in #1065.

[kt3k]

LGTM. Awesome work! @lowlighter @LionC

[LionC]

@lowlighter I think it would be a good idea to have it function more like the native shallow version aka assign, in a sense that it should mutate a target object and merge n objects instead of just two. That makes the function more flexible.

const mutatedA = deepAssign(a, b, c)

To create a new object

const d = deepAssign({}, a, b, c)

All functions in collections are pure right now and there is currently no plan to change that philosophy. I definitely see value in providing mutating functions (like filterInPlace which is used internally right now), especially for use in high-performance cases, but I think we would need to clearly distinguish them e.g. with their own subfolder.

[timreichen]

I am sorry to write this after it has already been merged. But I think deepMerge is misnamed, once because functions should start with a verb (made the mistake as well naming it deepAssign, sorry) and secondly because I think it can be generalized without loosing functionality.
Since it takes options why not call the function just merge and add an option for objects as well which will be set to merge by default as the others are to make it more general without loosing functionality?

[LionC]

I am sorry to write this after it has already been merged. But I think deepMerge is misnamed, once because functions should start with a verb (made the mistake as well naming it deepAssign, sorry) and secondly because I think it can be generalized without loosing functionality.
Since it takes options why not call the function just merge and add an option for objects as well which will be set to merge by default as the others are to make it more general without loosing functionality?

Because deepMerge is a very widely used package in the node space. While I might agree with your points in a vacuum, I think the concept of deepMerge is already so familiar for a lot of JS/TS devs, that it feels pragmatic to mirror that / piggyback on that knowledge.

[timreichen]

Yes, but even there the function name of the API is merge not deepMerge. deepmerge is just the package name. https://github.com/TehShrike/deepmerge#mergex-y-options