lodash/fp `mergeAll` and `mergeAllWith` does not have fixed arities and can miss customizer #4492

martingronlund · 2019-10-01T05:23:17Z

const customizer = console.log // returns undefined => merging is handled by `mergeAllWith`

// good
mergeAll([{ a: 1 }, { b: 2 }]) // { a: 1, b: 2 }
mergeAllWith(customizer, [{ a: 1 }, { b: 2 }]) // { a: 1, b: 2 }
// A-OK; customizer logs the following:
// undefined 2 "b" Object { a: 1, b: 2 } Object { b: 2 } undefined

// bad
mergeAll({}, { a: 1 }, { b: 2 }) // { a: 1, b: 2 }; OUCH, not fixed arity... IMO, update your docs. If you won't then at least do something like `if (!Array.isArray(arguments[0])) { return {} }`.

// ugly
mergeAll({ a: 1 }, { b: 2 }) // { b: 2 }; HUH?! OUCH!!! I bet this is an error in many systems out there... This is a no go. This must be fixed.
mergeAllWith(customizer, {}, { a: 1 }, { b: 2 }) // { a: 1, b: 2 } yay?
// hold your horses, this didn't log anything, meaning no customizer was used!! This could also be a frequent bug out there. Objects are being merged but in a possibly incorrect way! This must also be fixed.

jdalton · 2019-10-16T21:41:00Z

Hi @martingronlund!

The good is the expected usage. If you're up for making that more clear or have implementation ideas to restrict I'm open to it.

martingronlund · 2019-10-17T05:16:21Z

@jdalton hi!

I think wrapping in ary would make things a lot clearer.

const customizer = console.log

const mergeAll2 = fp.ary(1, fp.mergeAll)
const mergeAllWith2 = fp.ary(2, fp.mergeAllWith)

mergeAll2({}, { a: 1 }, { b: 2 }) // undefined
mergeAll2({ a: 1 }, { b: 2 }) // undefined
mergeAllWith2(customizer, {}, { a: 1 }, { b: 2 }) // {}, nothing logged
mergeAllWith2(customizer, { a: 1 }, { b: 2 }) // {}, nothing logged

It would make the actual output be much further from the expected output, thereby better alerting users that they're doing something wrong. I guess this could be considered either a patch or a major upgrade. SemVer is a bit broken in that regards. Systems out there which seem to work correctly 90% of the time might actually be faulty and end up working 0% of the time with this change, so IMO it should be a major upgrade, but I'm not even certain we have reached consensus here so maybe I'm getting ahead of myself.

I looked at the docs on https://github.com/lodash/lodash/wiki/FP-Guide and I'm confused by this:

Methods with a fixed arity of one:
... mergeAll ...

Methods with a fixed arity of two:
... mergeAllWith ...

I parse that as what I did above with ary should, according to documentation, already be done?

What do you think? I'm feeling ¯\(ツ)/¯ at this point

jdalton added the documentation label Oct 16, 2019

lodash / lodash

lodash/fp `mergeAll` and `mergeAllWith` does not have fixed arities and can miss customizer #4492

lodash/fp `mergeAll` and `mergeAllWith` does not have fixed arities and can miss customizer #4492

martingronlund commented Oct 1, 2019

jdalton commented Oct 16, 2019

martingronlund commented Oct 17, 2019 •

edited

lodash / lodash

Join GitHub today

lodash/fp `mergeAll` and `mergeAllWith` does not have fixed arities and can miss customizer #4492

lodash/fp `mergeAll` and `mergeAllWith` does not have fixed arities and can miss customizer #4492

Comments

martingronlund commented Oct 1, 2019

jdalton commented Oct 16, 2019

martingronlund commented Oct 17, 2019 • edited

martingronlund commented Oct 17, 2019 •

edited