TehShrike / deepmerge

A library for deep (recursive) merging of Javascript objects
MIT License
2.75k stars 216 forks source link

Default to only merging plain objects [Breaking] #168

Closed ArtskydJ closed 4 years ago

ArtskydJ commented 4 years ago

Fixes #152.

Changes the default isMergeableObject from https://github.com/TehShrike/is-mergeable-object to

var isPlainObj = require('is-plain-obj')

function defaultIsMergeableObject(value) {
    return Array.isArray(value) || isPlainObj(value)
}

Tests pass on my machine.

ArtskydJ commented 4 years ago

Uh oh, the test relies on the mutation of the options object, which appears to be going away? https://github.com/TehShrike/deepmerge/issues/166

I'm gonna see if I can change the code so it works without relying upon the options mutation.

ArtskydJ commented 4 years ago

Ok, I modified the commit so now the test does not rely upon the mutation of the options object.

TehShrike commented 4 years ago

I'd rather the tests weren't so aware of implementation details, and tested object identity in a way that would fail with the previous default implementation, e.g.

const input = {
    ohai: new Error()
}
const output = deepmerge({}, input)
assert(output.ohai === input.ohai) // should fail before your change, should pass after your change
ArtskydJ commented 4 years ago

I'd rather the tests weren't so aware of implementation details [SNIP]

Ah, makes sense. I should be able to take a look within a few days.

ArtskydJ commented 4 years ago

isMergeableObject has been changed to isMergeable.

The test has been changed to test the default behavior of the module instead of testing the isMergeable function on its own.