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

Custom array merging #37

Merged
merged 2 commits into from
Sep 29, 2016
Merged

Custom array merging #37

merged 2 commits into from
Sep 29, 2016

Conversation

TehShrike
Copy link
Owner

A potential fix for #14, #20, #21, #22, #24, #32.

The default merge strategy is exactly the same as it was before (though I'm suspicious of that falsey target property check on line 41 - that can be dealt with as a separate issue if need be).

You can implement whatever other array-merging strategy you like - the merge function is passed both arrays, plus the original options passed to deepmerge.

Any thoughts? Is this a good general solution? I didn't want to add complexity while solving this common issue.

I did a bit of refactoring in the first commit of this pull request, mostly to get rid of abbreviated variable names.

@KyleAMathews
Copy link
Collaborator

👍

@TehShrike
Copy link
Owner Author

I'll wait until the weekend to give any other contributors a chance to weigh in. If there aren't any other responses, I'll merge this and publish as a feature version bump, and close all the related pull requests and issues.

@macdja38
Copy link
Collaborator

this kinda destroys our ability to add an arbitrary number of arguments... #33

@TehShrike
Copy link
Owner Author

It keeps us from easily handling any number of arguments, but array merging behavior seems to be a much bigger deal for most users.

I think if there's enough user desire to pass in an arbitrary number of arguments, we can expose another method with a different API. Something like deepmerge.all([object1, object2, object3], optionsObject) perhaps. Assuming we agree that array merging is a higher priority, we can bikeshed #33 later.

@macdja38
Copy link
Collaborator

👍

@TehShrike TehShrike merged commit d34ab12 into TehShrike:master Sep 29, 2016
@TehShrike TehShrike deleted the custom-array-merging branch September 29, 2016 04:30
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 this pull request may close these issues.

3 participants