Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
yanickrochon opened this issue Jan 8, 2020 · 8 comments · May be fixed by #209
Open

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

yanickrochon opened this issue Jan 8, 2020 · 8 comments · May be fixed by #209

Comments

@yanickrochon
Copy link

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
Copy link
Owner

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
Copy link
Author

yanickrochon commented Jan 8, 2020

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

@yellow1912
Copy link

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
Copy link
Author

yanickrochon commented Mar 18, 2020

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
Copy link

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
Copy link

Is it at all possible to do this with deepmerge

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

d === a;   // -> true

@TehShrike
Copy link
Owner

deepmerge has never done that (though it probably should have) – see #186 (comment)

@RebeccaStevens
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants