Handle edge case where Arrays and Objects are merged

[macdja38]

Handle edge case where the library attempts to merge an array and an object.

merge({foo: {bar: true}}, {foo: ["bar"]})

This fix simply replaces the object with the array that is now at the objects location.

[KyleAMathews]

@macdja38 hey! I'd love to make you a collaborator so you can merge this and other PRs and roll a new release! Please signup over at #25 and let's make this happen!

[TehShrike]

@macdja38 could you add a new failing test for this issue? Or make a comment with the output you would expect from your example code above?

[macdja38]

@TehShrike I expect to get {foo: ["bar"]} out of the above merge as from what I understand to remain constant with the library's current behavior when the objects are not merge-able the 2nd argument should be applied over top of the first. I'll look into adding a failing test in the morning.

[TehShrike]

That makes sense to me.

As an aside, it would probably be better to use [Array.isArray](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray) instead of instanceof (isArray is already being used elsewhere in the module).

[macdja38]

@TehShrike I thought about that but Array.isArray can fail if the value is null or undefined though I suppose we check that before the code gets to that point.

[TehShrike]

Array.isArray(null) and Array.isArray(undefined) both return false, so we should be good.

[macdja38]

@TehShrike think you could just double check the test before one of us hits that big green merge button?

[TehShrike]

The test looks good to me.

Looking at it again, I wonder if we could make the "the target isn't an array" case even simpler - if the target isn't an array and we know we're just going to overwrite it, we don't need to iterate over it at all, we can just make a shallow copy and call it good, something like

if (Array.isArray(target)) {
    dst = dst.concat(target);
    src.forEach(function(e, i) {
        if (typeof dst[i] === 'undefined') {
            dst[i] = e;
        } else if (typeof e === 'object') {
            dst[i] = deepmerge(target[i], e);
        } else {
            if (target.indexOf(e) === -1) {
                dst.push(e);
            }
        }
    });
} else {
    dst = dst.concat(src)
}

buuuut, I'd kind of like to wait to merge this (or even think about changing the implementation to something like what I pasted above) until after #37 is merged, as one of the things that happens in that pull request is refactoring the default array merging behavior out into it's own function.

After #37 is merged, I think the implementation could look something like this:

if (array) {
   return Array.isArray(target) ? arrayMerge(target, source, optionsArgument) : source.slice()
} /* else ... */

[TehShrike]

I had another thought - what if the actual expected behavior was that merging { key1: { thingy: 'value' } } and { key1: [ 'array value' ] } would result in { key1: { thingy: 'value', '0': 'array value' } }?

That's not what the current behavior is though, so maybe it doesn't matter much.

[TehShrike]

(oh, and feel free to give #37 a review to help along its merging!)

[macdja38]

I think we should probably deploy this as soon as possible attempting the test on a current version of this library causes it to throw an exception.

    not ok 3 TypeError: target.indexOf is not a function
      ---
        type:    TypeError
        message: target.indexOf is not a function
        code:    ~
        errno:   ~
        stack:
          - |
            C:\Users\macdja38\deepmerge\index.js:31:31
          - Array.forEach (native)
          - |
            deepmerge (C:\Users\macdja38\deepmerge\index.js:26:13)
          - |
            C:\Users\macdja38\deepmerge\index.js:48:32
          - Array.forEach (native)
          - |
            deepmerge (C:\Users\macdja38\deepmerge\index.js:41:26)
          - |
            C:\Users\macdja38\deepmerge\index.js:30:26
          - Array.forEach (native)
          - |
            deepmerge (C:\Users\macdja38\deepmerge\index.js:26:13)
          - |
            Test.<anonymous> (C:\Users\macdja38\deepmerge\test\merge.js:28:17)
        thrown:  true
      ...

[TehShrike]

It's an awkward bug, but it's not a regression and it's been around for quite a while. I'm for getting it fixed, but I don't think we should feel rushed. Better to fix it right once than fix it one way now, have more discussions later, and decide to fix it a different way.