TehShrike / deepmerge

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

Restore "clone" feature via new option : mergeWithTarget #186

Open yanickrochon opened 4 years ago

yanickrochon commented 4 years ago

I have this use case :

import config from 'config.json';
import config_dev from 'config.dev.json';

if (env === 'dev') {
   merge( config, config_dev );
}

export default config;

my problem is that this module creates a destination object, and even if the docs has an old deprecated clone option, it has been removed anyway. I was expecting the same behavior as Object.assign but recursively with this module.

So, I propose to have this option re-inserted as a better alternative : mergeWithTarget with a default value of false.

Example usage:

import config from 'config.json';
import config_dev from 'config.dev.json';

if (env === 'dev') {
   merge( config, config_dev, { mergeWithTarget: true });
}

export default config;

Because sometimes there is no need to create 3 objects when 2 is perfectly acceptable.

TehShrike commented 4 years ago

One of the reasons is that clone is deprecated is because it's a lie – it always created a new destination object, even when you set clone to false.

It's still implemented, by the way – you just ran into the thing that it lies about.

The docs are technically correct:

If clone is false then child objects will be copied

Maybe in a future breaking change it would be worthwhile to re-instate the clone option but change its behavior so that if you set it to false, it would not create a new destination object. We'd have to revisit #71 at that point though.

yanickrochon commented 4 years ago

Instead of setting destination = {}, if it is set to target, it will effectively not create a copy at all.

yellow1912 commented 4 years ago

For many libraries including Vuejs that watches objects, running merge on the object may cut off the reactivity because the object is actually cloned. I think it's worth finding an option to support this non-clone kind of merging.

yanickrochon commented 4 years ago

Some library I did use once (can't remember which one) had this pattern

const d = merge(a, b, c);

d === a;   // -> true
const d = merge.clone(a, b, c);

d === a;  // -> false
yellow1912 commented 4 years ago

I think that is also nice. Either option though, it should be up to the developer to decide how the object is actually merged. There are use cases for both and in my situation I will have to switch to another plugin even though I really like this plugin (with the ability to control how array should be merged for example). This clone behavior create so much problem with reactivity inside Vuejs.

jakobrosenberg commented 3 years ago

Is it at all possible to do this with deepmerge

const d = merge(a, b, c);

d === a;   // -> true
TehShrike commented 3 years ago

deepmerge has never done that (though it probably should have) – see https://github.com/TehShrike/deepmerge/issues/186#issuecomment-572256858

RebeccaStevens commented 3 years ago

Looking at the original use case of this issue, this is how I would handle such a case:

const mergedConfig = env === 'dev' ? merge(config, config_dev) : config;

export default mergedConfig;

Though yes, a new mergeWithTarget option does seem like a good idea to me. The current behaviour of the clone option should still be keep as well though - maybe it just needs a more clear name?