TehShrike / deepmerge

A library for deep (recursive) merging of Javascript objects
MIT License
2.76k stars 216 forks source link

deepmerge not working with Set #202

Open itsdarrylnorris opened 4 years ago

itsdarrylnorris commented 4 years ago

Problem

I am running into a problem using the JavaScript Set function that after merging two objects for some reason it's not keeping the array from set.

Here is the code:

var merge = require("deepmerge");
var valuesWithSet = {
  testObject: new Set(["hello", "world"]),
};

console.log("valuesWithSet", valuesWithSet);
console.log(merge({}, valuesWithSet));

Output:

valuesWithSet { testObject: Set(2) { 'hello', 'world' } }
{ testObject: {} } // This should have array from set.

Solution

We need to keep values from set. The output should looks like this:

{ testObject: Set(2) { 'hello', 'world' } }
TehShrike commented 4 years ago

The current version of deepmerge is still using this naive object detection (typeof new Set() returns 'object').

You probably want to pass is-plain-obj in to isMergeableObject, per this part of the readme.

This will cause the set to be used directly on the new object (not cloned).

Related: #152 which is fixed in the v5 branch

Gaurav2048 commented 4 years ago

Can also use Array.from(#set) to get an array and then pass to deepmerge. #TehSrike do you suggest doing it in the library?

RebeccaStevens commented 4 years ago

@Gaurav2048 are you asking about having the library automatically convert sets to arrays? If so I don't think this should be supported as that would change the structure of the resulting object. I opened another issue (#204) a little while ago for handling Set and Map merging.

TehShrike commented 4 years ago

If you want that kind of behavior, you would probably want to use klona in your customMerge function.

RebeccaStevens commented 4 years ago

@TehShrike I think it might be worth having a customClone option in addition to the customMerge option. This library's current built in clone method fails when it tries to clone non plain objects. The option would also address the concerns raised in #163.

TehShrike commented 4 years ago

There might be traps in there somewhere but my initial reaction is that makes sense to me