Arrays with similar items merge in an unexpected manner

[basicallydan]

Great library! I have a small issue with a certain behaviour though. Maybe it's on purpose.

var merge = require('../')
var x = { foo : { 'bar' : 3 }, ar : [ { thing : 'b' } ] }
var y = { foo : { 'baz' : 4 }, quux : 5, ar : [ { thing : 'a' } ] }
var merged = merge(x, y)
console.dir(merged)

I would expect merged to look like this:

{ foo: { bar: 3, baz: 4 }, ar: [ { thing : 'b' }, { thing: 'a' } ], quux: 5 }

However it looks like this:

{ foo: { bar: 3, baz: 4 }, ar: [ { thing: 'a' } ], quux: 5 }

To me the behaviour I expect makes more sense because an array is likely to be a set of similar objects with shared keys but different values for those keys. However, I understand if that is not one of the behaviours that you would expect with the library so I shall make my own fork, and if you agree with my expected behaviour, I'll do a pull request for it 😄

[vvo]

Your behavior seems legit, if tests are ok submit a PR and ping @nrf110

[howardroark]

I'm wondering if merging arrays is a good idea? I mean... it seems like a slippery slope. Might be more sane to just take the entire array from "object y". I think the current behaviour is correct.

[howardroark]

Oh wait... it does do it for digits. Hmmmm... I don't know if that makes sense.

To me it should only be key/value merging. If you accept the "y" value of two matching keys when they have a string value... why merge them when they are arrays? What if one is a string and one is an array?

[dkniffin]

I would like to see this issue resolved. I'm running into a similar issue:

var meh1 = { '1': [ { key: 'wires', value: '*' } ] }
var meh2 = { '1': [ { key: 'place', value: 'village' } ] }
console.log(merge(meh1,meh2))
=> { '1': [ { key: 'place', value: 'village' } ] }

I expect:

{ '1': [ { key: 'wires', value: '*' }, { key: 'place', value: 'village' } ] }

@basicallydan Can you create a pull request for your fork?

EDIT: Confirmed that the fork works for my issue.

[basicallydan]

Hey @oddityoverseer13, it does work indeed... I remember there was a reason I didn't make a PR, I forget exactly what it was though. I'll have a look outside of working hours this week :)

[howardroark]

Just add my 2 cents here...

I would expect this tool to only merge objects and leave arrays alone. To me arrays are too ambiguous and force the tool to start making assumptions when combining them. I also think that if you have a properly designed object schema you would not really have the need to merge arrays. Rather you would expect one object's array value to trump another.

Though I assume this tool is being used to merge data that is clearly defined.

[basicallydan]

I agree with @howardroark, despite me being the person to open this up.

It might be nice to make the array-behaviour ability based on an option, e.g. - should it add to the array, replace it entirely or replace selected indices. E.g.

merge(meh1,meh2, { arrays: 'add' });
merge(meh1,meh2, { arrays: 'replace' });
merge(meh1,meh2, { arrays: 'replaceIndex' });

Something like that?

[dkniffin]

@basicallydan That sounds like a good option. Give the users the flexibility to do what they'd like.

[mistic100]

If some jQuery users are interested I created a function similar to $.extend with configurable behaviour for arrays: https://github.com/mistic100/jQuery.extendext

These behaviours are:

• merge like objects (jQuery default)
• replace (for @howardroark)
• concat (like @basicallydan new option)
• extend (how deepmerge currently works)

[nhocki]

@basicallydan thanks for your fork, it worked for me. If you need a hand with the changes you proposed let me know, I can give you a hand.

[bdefore]

i was just surprised by this too. objects with the same name of type array i expected to concatenate and were replaced by the second one:

a =
  foo:
    bar: [
      {
        baz: "12"
      }
      {
        baz: "34"
      }
    ]
b =
  foo:
    bar: [
      {
        baz: "56"
      }
      {
        baz: "78"
      }
    ]
c = merge(a,b)

for item in c.foo.bar
  console.log 'item', item

# output:
# item { baz: '56' }
# item { baz: '78' }

[vyorkin]

I've just tried to use deepmerge to merge my env-specific webpack configs:

> baseConfig
{ module: { loaders: [ [Object] ] } }

> envConfig
{ module: { loaders: [ [Object] ] } }

> deepmerge(baseConfig, envConfig);
{ module: { loaders: [ [Object] ] } }

for me it was completely unexpected behaviour

[gyzerok]

Have the same issue here. Is there plans to resolve this or I need to create my own implementation?

[anthonator]

I don't think how array merges are currently implemented is incorrect. I also don't think concatenating arrays is incorrect behavior either. Both use cases are valid. That's why this is tricky.

What makes the most sense to me is to allow the user to decide how they want their arrays handled. Adding a third options parameter to deepmerge might be a viable option.

var obj1 = { a: [{ b: 'c' }] }
var obj2 = { a: [{ d: 'e' }] }
deepmerge(obj1, obj2) // { a: [{ b: 'c', d: 'e' }] }

var obj1 = { a: [{ b: 'c' }] }
var obj2 = { a: [{ d: 'e' }] }
deepmerge(obj1, obj2, { arrays: 'concat' }) // { a: [{ b: 'c' }, { d: 'e' }] }

[anthonator]

or this

#21

[vvo]

What makes the most sense to me is to allow the user to decide how they want their arrays handled. Adding a third options parameter to deepmerge might be a viable option.

Yes lodash deepmerge does nothing about array by default but you can do it by providing a custom fn that check if it's an array: https://lodash.com/docs#merge

I think I was the one that did the "funny" array merge here, it really depends on your use case, that's why it's tricky as you said. Allowing an option and maybe reverting to the default that the first issue comment sayd would be better (+ major bump).

[vvo]

Anyone can do it here and propose the change

[vvo]

9180a6d

[anthonator]

Lodash has a deep extend extension called extendify that has a sane implementation of array merge strategies. Might make sense to check that out for inspiration.

[TehShrike]

Sounds like some folks already reached the same thought as I did in #32 (comment) - allowing consumers to pass in a custom array-merging function seems like a solid idea to me.

[TehShrike]

Looks like lots of folks had array merging issues. They account for pull requests #20, #21, #22, #32

[TehShrike]

And issue #24!

[TehShrike]

I opened pull request #37 to solve this issue - comment there if you have any thoughts.