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

Is it possible to opt-out of array merging? #24

Closed
amannn opened this issue Nov 3, 2015 · 7 comments
Closed

Is it possible to opt-out of array merging? #24

amannn opened this issue Nov 3, 2015 · 7 comments

Comments

@amannn
Copy link

amannn commented Nov 3, 2015

It's a really nice feature, but sometimes it's not the right thing.

Or is there a quick fix how to prevent the merging of all array items, but rather replace the array with the one provided as the second argument?

@TehShrike
Copy link
Owner

This seems to be the most common request by far. Check out this comment #32 (comment) and chime in on #14.

@TehShrike
Copy link
Owner

If #37 is merged, you could achieve your purpose with this code:

function useSource(destinationArray, sourceArray) {
  return sourceArray
}
var output = deepMerge(destination, source, { arrayMerge: useSource })

Please comment on #37 if you have an opinion.

@adamreisnz
Copy link

Uhm, why isn't this the default behaviour when we pass in false as arrayMerge? Seems redundant to have to pass a custom function to get this outcome.

@adamreisnz
Copy link

adamreisnz commented Aug 17, 2017

@TehShrike Are you open to a PR that introduces arrayMerge: false as a valid option?

@TehShrike
Copy link
Owner

TehShrike commented Aug 17, 2017

I'd rather not change the signature to make boolean types valid for arrayMerge in addition to functions. What would passing in true mean?

Is this so bad?

const dontMerge = (destination, source) => source
const output = deepMerge(destination, source, { arrayMerge: dontMerge })

Perhaps this use case just needs to be documented in the readme?

@adamreisnz
Copy link

adamreisnz commented Aug 17, 2017

It's worse, not DRY, and needlessly verbose in my opinion, compared to

const output = deepMerge(destination, source, { arrayMerge: false })

If you use deepmerge a lot all over your code base, you're going to have to create that dontMerge function all over the place, or stash it in a helper somewhere and remember to require it every time you require deepmerge. Just seems like a hassle.

I'm not sure where your worry for changing the signature comes from, but as javascript is not a strongly typed language why not take advantage of the fact and allows support for false. It doesn't mean you also need to add support for true, and you can document it properly.

A simple 3 lines of code could handle it:

if (options.arrayMerge === false) {
  options.arrayMerge = (destination, source) => source;
}

It would just convert the false value in the options to the appropriate "no change" handler, so that the rest of the codebase can remain unchanged and unaffected.

@TehShrike
Copy link
Owner

Allowing for different types of option values is certainly valid, but it increases the complexity of the API.

The job of libraries like this is to expose a sensible API. There are a hundred different use cases for merging, I'm not going to add feature bloat to favor some of them, even if it's only a few lines at a time.

Check out how many issues were closed by #37 - imagine what the code base would look like if each consumer's case was handled individually.

DRY is still within your grasp - as you said, if you want the same set of options everywhere by default, you can create a module in your app's codebase that defaults to those options.

TehShrike added a commit that referenced this issue Aug 24, 2017
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

No branches or pull requests

3 participants