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

Variable number of arguments #33

Closed
monolithed opened this issue Jun 30, 2016 · 15 comments
Closed

Variable number of arguments #33

monolithed opened this issue Jun 30, 2016 · 15 comments

Comments

@monolithed
Copy link

It would be nice if deepmerge will support a viariable number of arguments

@TehShrike
Copy link
Owner

That would make sense to me, too. It might be a bit trickier to add though, since so many people seem to want the ability to configure array merging via a third argument (#14).

It's probably still possible, though. I wouldn't be against a pull request.

@TehShrike
Copy link
Owner

If anyone wants to suggest a backwards-compatible API and/or open a pull request, fire away.

@zspecza
Copy link

zspecza commented Nov 1, 2016

While this option may be convenient, it's not too difficult to just write a wrapper around deepmerge on a case-by-case basis:

const deepMerge = require("deepmerge")

const deepMergeAll = (...xs) => {
  const initial = xs.shift()
  return xs
    .filter((x) => typeof x !== 'undefined' && x !== null)
    .reduce((x, y) => deepMerge(x, y), initial || {})
}

deepMergeAll({ a: 1 }, { b: 2 }, null, { c: 3 })
// { a: 1, b: 2, c: 3 }

I don't think there are very many projects that use more than a maximum of 5 different array merge strategies (that just sounds super rare & unrealistic, imo) so as far as boilerplate goes, this isn't too bad.

@Bloomca
Copy link
Contributor

Bloomca commented Nov 1, 2016

Well, it is possible just to expose another function, as @declandewet suggested.

@TehShrike
Copy link
Owner

How do y'all feel about an API like const merged = deepmerge.options({ clone: true })(a, b, c, d, e)?

const mergeAll = deepmerge.options({ arrayMerge: someFunction })

const mergedA = mergeAll(a, b, c)
const mergedB = mergeAll(b, c, a)

@monolithed
Copy link
Author

monolithed commented Nov 1, 2016

My suggestion:

merge([x, y, z], {  /* options */  });

@TehShrike
Copy link
Owner

I like that, though I don't think we could distinguish between that and somebody trying to merge a regular object into a normal array. We'd have to add a new method, merge.all([x, y, z], { /* options */ }) or something.

@zspecza
Copy link

zspecza commented Nov 1, 2016

You can't merge a Boolean, so you could inspect if the first argument is boolean and use that as a hint that the 2nd param is an options object and return a function that merges all arguments

const deepMergeAll =  deepmerge(true, [options])

const merged = deepMergeAll(a, b, c, d)

This would be just as convenient and mitigate any need for a breaking change

or perhaps a string might be more descriptive

const deepMergeAll =  deepmerge('all', [options])

@TehShrike
Copy link
Owner

Adding a new function like merge.all or merge.options wouldn't be a breaking change, just a feature version bump.

Relying on special values in ordered arguments makes me nervous.

@Bloomca
Copy link
Contributor

Bloomca commented Nov 1, 2016

I feel the same as @TehShrike – there is no particular need to break things, and passing Boolean looks very esoteric. I feel that new function is the most "soft" solution here.

Though I am not sure about instantiating new function with different merge strategies – it looks kind of strange too (at least for me). I can't really suggest better solution – only the first argument, and then collect other values through rest spread, but then we make first argument mandatory.

@TehShrike
Copy link
Owner

Vote for your preference via reactions (assume that the names are changeable, so vote just based on api/functionality):

Option ❤️

const merge = require('deepmerge')

const mergeAll = merge.options({ clone: true })

const mergedObject = mergeAll(a, b, c)

Option 🎉

const merge = require('deepmerge')

const mergedObject = merge.all([a, b, c], { clone: true })

@TehShrike
Copy link
Owner

Sounds good to me! We'll go with merge.all([a, b, c], { clone: true }).

Unless anyone thinks of an objection, I'll say the specs are something like:

  • Options object should be optional
  • An error should be thrown if the first argument is not an array of at least two elements

Pull requests with tests for these checks, or tests for the rest of the functionality, would be much appreciated. I think they should go into a new file test/merge-all.js.

@Bloomca
Copy link
Contributor

Bloomca commented Nov 2, 2016

@TehShrike I'd like to do this! I can do this on weekend.

@macdja38
Copy link
Collaborator

macdja38 commented Nov 3, 2016

This sounds Like a very good way to go about this!

@TehShrike
Copy link
Owner

This is implemented by #50 and published as 1.3.0!

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

5 participants